On Wed, 18 Jun 2025 10:16:41 GMT, Jayathirth D V <j...@openjdk.org> wrote:

>> Metal is here! Yes!
>> 
>> It took me a while to sort out how the GlassView and GlassLayer classes work 
>> in this PR. It’s not clear based on the naming that the classes which handle 
>> drawing (GlassViewCGL3D and GlassViewMTL3D ) are not subclasses of the class 
>> that handles events (GlassView3D). The naming conventions could be clearer 
>> or at the very least there should be some comments in the code.
>> 
>> I think some of this structure is being dictated by the way GlassViewCGL3D 
>> is derived from NSOpenGLView since that prevents us from making it a 
>> subclass of, say, GlassView3D. But I’m almost certain the NSOpenGLView 
>> reference is just cruft. All of the OpenGL processing happens in a 
>> CAOpenGLLayer which has nothing to do with the (unused) NSOpenGLView 
>> machinery. In the current master and this PR I can derive from NSView 
>> instead of NSOpenGLView and everything works fine.
>> 
>> There is some code in Prism (MacOSXWindowSystemInterface.m) that can 
>> associate an NSOpenGLContext with an NSView but I don’t think a view is ever 
>> passed in. It would be nice to clean that code up since the view-related 
>> calls are deprecated and generating compiler warnings.
>> 
>> I think the NSOpenGLView reference should be removed. Beyond that you could 
>> keep the current class structure and view layout since there’s something to 
>> be said for separating drawing and event processing into different NSViews.
>> 
>> I would rather not see the 3D terminology carried over into the new classes. 
>> It's there to make a distinction that's no longer relevant (according to the 
>> comments there used to be a GlassView2D). But that is a personal pet peeve 
>> so feel free to ignore it.
>> 
>> Look like it’s not possible to add comments to unchanged code in GitHub (?!) 
>> so I guess I have to write the following issues up here.
>> 
>> In GlassWindow.m line 807 there’s this bit of mysterious code:
>> 
>> 
>> CALayer *layer = [window->view layer];
>> if (([layer isKindOfClass:[CAOpenGLLayer class]] == YES) &&
>>     (([window->nsWindow styleMask] & NSWindowStyleMaskTexturedBackground) == 
>> NO))
>> {
>>     [((CAOpenGLLayer*)layer) setOpaque:[window->nsWindow isOpaque]];
>> }
>> 
>> 
>> It was put there to resolve 
>> [JDK-8095004](https://bugs.openjdk.org/browse/JDK-8095004) and 
>> [JDK-8095040](https://bugs.openjdk.org/browse/JDK-8095040). In this PR the 
>> layer is nil at this point so this code isn’t doing anything. I haven’t 
>> verified whether this is causing a regression. In the master branch that 
>> setOpaque: call will always happen since the NSWindowSt...
>
> @beldenfox Thanks for your inputs.
> 
> 1.  Regarding adding comments in GlassView/Layer classes about how they are 
> not subclasses but subViews/subLayers : Sure i will go ahead and add comments 
> in the class. Also as you mentioned we can name them better:
> 
> - GlassView3D ->  GlassViewEventHandler(Since it is common class which 
> handles events for both OpenGL and Metal pipeline) Or GlassSubView(Since it 
> is added as subView in GlassHostView in GlassView.m). Please suggest if you 
> have better names that we can use here.
> - GlassViewCGL3D -> GlassViewCGL(With comment mentioning how it is not 
> subclass but a subView of GlassView3D)
> - GlassViewMTL3D -> GlassViewMTL(With comment mentioning how it is not 
> subclass but a subView of GlassView3D)
> - At other places also i will remove 3D in names and appropriate comment in 
> CGL/MTL layer class.
> 
> 2. Regarding NSOpenGLView : Since both CGL and MTL views are not subclass of 
> same NSView we have current structure with Event handling in common class 
> GlassView3D and CGL & MTL Views as subViews of GlassView3D. I also tried 
> removing NSOpenGLView reference and then running Ensemble8 in ES2 pipeline 
> and things work fine. But we want to minimize changes related to already 
> present default ES2 pipeline in this PR as part of refactoring effort. We can 
> work on it in as a follow-up issue.
> 
> 3. Regarding change in MacOSXWindowSystemInterface.m : Same here we want to 
> minimize changes related to ES2 in this PR. We can work on it in as a 
> follow-up issue.
> 
> 4. Regarding setOpaque() in GlassWindow.m : While refactoring Glass for Metal 
> pipeline under [JDK-8325379](https://bugs.openjdk.org/browse/JDK-8325379) 
> this was identified as follow up issue : 
> [JDK-8356758](https://bugs.openjdk.org/browse/JDK-8356758). We were aware of 
> how this code related to 
> [JDK-8095004](https://bugs.openjdk.org/browse/JDK-8095004) but thanks for 
> pointing to [JDK-8095040](https://bugs.openjdk.org/browse/JDK-8095040) which 
> was not verified. There are no regression seen with this refactoring, 
> PickTest3D works properly in both ES2 and MTL pipeline of this PR and matches 
> output seen with mainline code. I verified that test mentioned in 
> [JDK-8095040](https://bugs.openjdk.org/browse/JDK-8095040) on latest mainline 
> code and i still see that black filling is drawn. Same behaviour is seen with 
> ES2 and MTL pipeline with this PR. But it works properly in 8u431, looks like 
> the issue has resurfaced so i have created new bug : 
> [JDK-8359904](https://bugs.openjdk.org/browse/JDK
 -8359904)
> 
> 5. Regard...

@jayathirthrao Thanks for the thorough response. And, again, it's great to see 
Metal happening.

> * GlassView3D ->  GlassViewEventHandler(Since it is common class which 
> handles events for both OpenGL and Metal pipeline) Or GlassSubView(Since it 
> is added as subView in GlassHostView in GlassView.m). Please suggest if you 
> have better names that we can use here.

I want to keep the GlassView prefix since these classes implement the platform 
side of the View class in the toolkit. If we were to rename anything it would 
be GlassViewCGL and GlassViewMTL since these are more closely related to Prism 
than the toolkit View class. But I'm overthinking things. For now I think we 
should keep GlassView and GlassView3D along with GlassViewCGL and GlassViewMTL 
and just clarify what's going on in the comments. At some point I want to 
combine GlassView and GlassView3D into one class but that can wait for a 
follow-up issue.

> 4. Regarding setOpaque() in GlassWindow.m

In my testing the setOpaque call isn't happening in this PR since the layer 
hasn't been created yet. But it doesn't seem to matter (?)

I'm having a hard time reproducing the original PickTest3D bug in the master 
branch. The screenshots in the bug report show translucency but the original 
bug report doesn't mention it and the PickTest3D code doesn't call for it. I 
tweaked PickTest3D to try a few combinations of setOpacity and 
DECORATED/TRANSPARENT and that setOpaque call didn't seem to make any 
difference even in the master branch. I must be missing something.

As for the UNIFIED issue I don't think there's a bug. The implementation relied 
on the OS to draw a brushed metal background but Apple removed that years ago 
and now just draws black. I will take a look but only because I want to 
understand how this works; we might need similar logic if we want to support 
desktop translucency effects. UNIFIED is deprecated and doesn't work on Windows 
either so we should probably close the new bug as Never Fix.

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1824#issuecomment-2985583069

Reply via email to