On Wed, 16 Jul 2025 14:59:07 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> you might want to sync up with the latest master.
Agreed, although you might wait until after the jfx25 fork at this point.
> modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2SwapChain.java
> line 53:
>
>> 51: // a value of zero corresponds to the windowing system-provided
>> 52: // framebuffer object
>> 53: long nativeDestHandle = 0;
>
> `= 0` is not strictly necessary.
But it isn't harmful. More to the point, this PR tries to minimize changes to
existing code, and removing the `= 0` would be an unrelated change.
> modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2SwapChain.java
> line 240:
>
>> 238: @Override
>> 239: public int getFboID() {
>> 240: return (int)nativeDestHandle;
>
> this results in loss of information. any possibility that it may backfire
> somehow, by mapping two different `nativeDestHandle`s to the same `int`?
Good question. Given that the ES2 pipeline only ever uses an int value, it's
_probably_ fine. But it's hard to prove without looking at the native code. At
the very least, a comment explaining this would be good.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line
> 57:
>
>> 55: shaderMap.put(fragmentFunctionName, this);
>> 56: } else {
>> 57: throw new AssertionError("Failed to create Shader");
>
> is `AssertionError` the right type here?
Good catch. I think this should throw a `RuntimeException` of some sort.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1824#issuecomment-3083891955
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211384171
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211407796
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211394358
