8373232: Set default filters and addressing modes for 3D metal textures #2005

jayathirthrao · 2025-12-12T08:18:14Z

Currently we don't set any texture filtering & addressing modes for 3D textures in Metal pipeline.
Default 3D texture filtering is "nearest" for Metal textures, but we use "linear" filtering by default for 3D textures in case of OpenGL.
Default addressing mode is "clamp_to_edge" in Metal textures, but we use "repeat" addressing mode for 3D textures in OpenGL.

Metal should use same default filtering & addressing modes for 3D textures as in OpenGL pipeline.
Under this issue we are trying only to match defaults of Metal to OpenGL pipeline.

We don't update the filters/addressing modes for 3D textures once created.
We have open enhancements like https://bugs.openjdk.org/browse/JDK-8324594 where we might provide API's to set sampler states for 3D textures in future.

Functional and performance testing is green with this update.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Integration blocker

⚠️ Title mismatch between PR and JBS for issue JDK-8373232

Issue

JDK-8373232: Pass default filters and addressing modes for 3D metal textures (Bug - P4) ⚠️ Title mismatch between PR and JBS.

Reviewers

Lukasz Kostyra (@lukostyra - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2005/head:pull/2005
$ git checkout pull/2005

Update a local copy of the PR:
$ git checkout pull/2005
$ git pull https://git.openjdk.org/jfx.git pull/2005/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2005

View PR using the GUI difftool:
$ git pr show -t 2005

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2005.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-12-12T08:19:38Z

👋 Welcome back jdv! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

jayathirthrao · 2025-12-12T08:21:02Z

modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPhongMaterial.java

            hTexture = mtlTex.getNativeHandle();
        }
-        context.setMap(nativeHandle, map.getType().ordinal(), hTexture);
+        context.setMap(nativeHandle, map.getType().ordinal(), hTexture, useMipmap);


We use mipmapping only when (!PlatformUtil.isEmbedded()) && (i == PhongMaterial.DIFFUSE || i == PhongMaterial.SELF_ILLUM);. We need to pass mipmap information, so that we can pick appropriate sampler state in native code.

openjdk · 2025-12-12T08:21:27Z

@jayathirthrao This change is no longer ready for integration - check the PR body for details.

mlbridge · 2025-12-12T08:27:05Z

Webrevs

01: Full - Incremental (7fb54da4)
00: Full (80fdcc8c)

lukostyra

In general looks good, but I have one question to double-check something I couldn't find.

The sampler is chosen based on whether we use mipmaps or not. In cases where mipmaps are not used, are we still generating them?

jayathirthrao · 2025-12-15T04:31:13Z

In general looks good, but I have one question to double-check something I couldn't find.

The sampler is chosen based on whether we use mipmaps or not. In cases where mipmaps are not used, are we still generating them?

@lukostyra Thanks for the review.
We use mipmap only for diffuse and self illumination maps : https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPhongMaterial.java#L100

We pass this mipmap flag to the native code when texture is getting created. And we use this flag and generate mipmaps only for the textures where this flag is true : https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-prism-mtl/MetalTexture.m#L203

So for non-mipmapped 3D textures we don't generate mipmaps.

lukostyra

Thanks for responding. I found one thing that could be improved, will run some tests on this later on.

modules/javafx.graphics/src/main/native-prism-mtl/MetalContext.m

lukostyra

LGTM

lukostyra · 2025-12-16T16:15:58Z

I just noticed the ready label, but I think this requires two reviewers.

/reviewers 2

openjdk · 2025-12-16T16:16:50Z

@lukostyra
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

arapte

Leaving a few queries regarding the changes.
I expect there should not be any behavior changes, correct ?

arapte · 2025-12-17T06:57:33Z

modules/javafx.graphics/src/main/native-prism-mtl/MetalContext.m

+/*
+ * Class:     com_sun_prism_mtl_MTLContext
+ * Method:    nSetDeviceParametersFor3D
+ * Signature: (J)J


minor : (J)J should be changed to (J)V

arapte · 2025-12-17T07:21:59Z

modules/javafx.graphics/src/main/native-prism-mtl/MetalMeshView.m

+    [phongEncoder setFragmentSamplerState:[material getSamplerState:DIFFUSE]
+                                  atIndex:0];
    [phongEncoder setFragmentTexture:[material getMap:SPECULAR]
                             atIndex:1];
+    [phongEncoder setFragmentSamplerState:[material getSamplerState:SPECULAR]
+                                  atIndex:0];
    [phongEncoder setFragmentTexture:[material getMap:BUMP]
                             atIndex:2];
+    [phongEncoder setFragmentSamplerState:[material getSamplerState:BUMP]
+                                  atIndex:0];
    [phongEncoder setFragmentTexture:[material getMap:SELFILLUMINATION]
                             atIndex:3];
+    [phongEncoder setFragmentSamplerState:[material getSamplerState:SELFILLUMINATION]
+                                  atIndex:0];


All setFragmentSamplerState are setting at index 0, should it not be 0,1,2,3?

Does this not require any changes to PhongPS.metal, The shader functions in PhongPS.metal do not have a sampler parameter.

Currently the PhongPS.metal file creates two samplers in shader file itself and uses those for sampling. May be that should be modified to use these samplers that are set using setFragmentSamplerState

Thanks for the observations @arapte .
Yes these changes are needed and it also has to be tested properly. So moving this PR back to draft stage.

bridgekeeper · 2026-02-11T22:06:40Z

@jayathirthrao This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

jayathirthrao added 4 commits December 8, 2025 16:08

8373232: Set default filters for 3D metal textures

1f6e24a

Use set device parameters for 3D call

a8ed468

force MTL

64ddd46

revert default MTL

80fdcc8

jayathirthrao commented Dec 12, 2025

View reviewed changes

openjdk bot added the rfr Ready for review label Dec 12, 2025

lukostyra reviewed Dec 12, 2025

View reviewed changes

lukostyra reviewed Dec 16, 2025

View reviewed changes

modules/javafx.graphics/src/main/native-prism-mtl/MetalContext.m Show resolved Hide resolved

create sampler states only once

7fb54da

lukostyra approved these changes Dec 16, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Dec 16, 2025

openjdk bot removed the ready Ready to be integrated label Dec 16, 2025

arapte suggested changes Dec 17, 2025

View reviewed changes

jayathirthrao marked this pull request as draft December 17, 2025 08:53

openjdk bot removed the rfr Ready for review label Dec 17, 2025

8373232: Set default filters and addressing modes for 3D metal textures #2005

Are you sure you want to change the base?

8373232: Set default filters and addressing modes for 3D metal textures #2005

Uh oh!

Conversation

jayathirthrao commented Dec 12, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Integration blocker

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Dec 12, 2025

Uh oh!

jayathirthrao Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

lukostyra left a comment

Choose a reason for hiding this comment

Uh oh!

jayathirthrao commented Dec 15, 2025

Uh oh!

lukostyra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukostyra left a comment

Choose a reason for hiding this comment

Uh oh!

lukostyra commented Dec 16, 2025

Uh oh!

openjdk bot commented Dec 16, 2025

Uh oh!

arapte left a comment

Choose a reason for hiding this comment

Uh oh!

arapte Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

arapte Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

jayathirthrao Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Feb 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

jayathirthrao commented Dec 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Dec 12, 2025 •

edited

Loading

mlbridge bot commented Dec 12, 2025 •

edited

Loading