RFR: 8092272: [D3D 3D] Need a robust 3D states management for texture

[Michael Strauß]

On Wed, 8 Nov 2023 22:30:34 GMT, Nir Lisker <nlisker at 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