On Wed, 8 Nov 2023 22:30:34 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
> Moves the filter setting of the samplers from the device parameters
> configuration to the use-site, allowing for dynamic changes in the sampler.
> This PR does internal plumbing work only to bring it close to the ES2
> pipeline. A followup PR will create the public API.
>
> Summary of the changes:
> * Created a new (internal for now) `TextureData` object that is intended to
> contain all the data of texture (map) of `PhongMaterial`, such as filters,
> addressing, wrapping mode, mipmaps etc. **This PR deals only with filters**
> as a starting point, more settings can be added later.
> * Creates an update mechanism from the Java side material to the native D3D
> layer. The public API `PhoneMaterial` is *not* changed yet. The peer
> `NGPhongMaterial` is configured to receive update from the public
> `PhongMaterial` when the public API is created via new
> `ObjectProperty<TextureData>` properties.
> * Small refactoring in the D3D layer with a new map types enum to control the
> texture settings more easily.
>
> The JBS issue lists some regressions in a comment, but I couldn't reproduce
> them. It looks like the sampler settings needed to be added anywhere, and
> that was the easiest to do at the time. Now they were just moved.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/paint/TextureData.java
line 12:
> 10: */
> 11: // Here we can support mipmaps, wrapping modes (which exists internally
> and can be pulled out), addressing modes etc.
> 12: public class TextureData {
1. This type could be a record.
2. There's no actual texture data (i.e. pixels) here, maybe a better name would
be `SamplerParameters` or something like that.
modules/javafx.graphics/src/main/native-prism-d3d/D3DContext.cc line 357:
> 355:
> 356: enum minmagFilterType { nearest_point, bilinear };
> 357: enum mipmapFilterType { none, nearest, linear };
These two types don't seem to be used anywhere.
modules/javafx.graphics/src/main/native-prism-d3d/D3DContext.cc line 369:
> 367: {
> 368: static const D3DTEXTUREFILTERTYPE minMagEnumMap[2] = {
> D3DTEXF_POINT, D3DTEXF_LINEAR };
> 369: static const D3DTEXTUREFILTERTYPE mipmapEnumMap[3] = { D3DTEXF_NONE,
> D3DTEXF_POINT, D3DTEXF_LINEAR };
You could use `std::array` as a safer alternative for plain arrays.
modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 295:
> 293: for (int i = 0; i < map_type::num_map_types; i++) {
> 294: map_type type = static_cast<map_type>(i);
> 295: SUCCEEDED(device->SetTexture(type, material->getMap(type)));
Wrapping the calls with the `SUCCEEDED` macro doesn't do anything useful.
modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.cc line 39:
> 37: for (int i = 0; i < map_type::num_map_types; i++) {
> 38: map[i] = NULL;
> 39: }
The size of the array is statically known, you could use `memset(map, 0,
sizeof(map))` (or, if you use `std::array`, it's simply `map.fill(NULL)`).
However, why is there a destructor at all? It doesn't seem to do anything
useful.
modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.cc line 70:
> 68:
> 69: bool D3DPhongMaterial::isBumpMap() {
> 70: return map[map_type::bump] ? true : false;
Since `map` contains pointers, you might consider `return map[map_type::bump]
!= NULL`.
modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.h line 41:
> 39: self_illumination,
> 40: num_map_types
> 41: };
1. Types generally seem to use PascalCase in this library, and constants
UPPER_CASE or CamelCase.
2. You can use `enum class` to prevent the constants from polluting the global
namespace.
3. The underlying type doesn't need to be specified, it's `int` by default.
You're casting it to `int` in `D3DMeshView.cc` anyway.
modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.h line 57:
> 55: bool isSpecularColor();
> 56: bool isSelfIllumMap();
> 57: IDirect3DBaseTexture9 * getMap(map_type type);
Space between `*` and `getMap`.
modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.h line 67:
> 65: float specularColor[4] = {1, 1, 1, 32};
> 66: bool specularColorSet = false;
> 67: IDirect3DBaseTexture9 *map[map_type::num_map_types] = {NULL};
You could use `std::array` as a safer alternative for plain arrays.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387437548
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387410882
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387443618
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387437011
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387420999
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387431297
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387419835
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387412857
PR Review Comment: https://git.openjdk.org/jfx/pull/1281#discussion_r1387429857
