GUI Control opacity regression in 8.21.0 when calling moveToVector3

Bugs
1

We hit an edge case with this commit: Fix GUI 2D optimization and consistently apply states when measuring … · BabylonJS/Babylon.js@20fd25b · GitHub

When setting AllowAlphaInheritance in combination with calling moveToVector3 (which calls _moveToProjectedPosition internally) on more than one GUI control, because the context is not saved, the globalAlpha (changed in _applyStates) will start to affect other controls.

Repro playground: https://playground.babylonjs.com/#DTH2XR#2

I’ve added a crude fix by saving/restoring the context in the _processMeasures function, which can be toggled with the APPLY_FIX variable. This isn’t a good fix though as it duplicates the context.save/restore calls in _layout. The fix we applied internally is to run context.save/restore around the processMeasures call in moveToProjectedPosition.

Thanks for your work in this project!

2

cc @amoebachant

3

Hi @lwp-joel - thanks for reporting this and for the suggested fix! I actually think it’s a better fix than you said :slight_smile:

Context .save() and restore() makes use of a stack, so having _processMeasures() contain a pair of save() / restore() calls should restore it to the prior behavior where _processMeasures() didn’t call _applyStates(). Now _processMeasures() will correctly save()/restore() internally, which is required when calling applyState(), but won’t affect the state of it’s caller.

I’m testing this out now and will publish the PR soon. Thanks for the bug report!

4

Thanks for your work on this!

so having _processMeasures() contain a pair of save() / restore() calls should restore it to the prior behavior where _processMeasures() didn’t call _applyStates()

Unfortunately I discovered that this actually introduces new layout issues with our code (BabylonJS also prints out layout errors to the console). These errors didn’t exist prior to 8.21.0 either, so I believe it would introduce a new regression.

I didn’t track down why exactly these layout issues were triggered, but setting the context.save/restore calls in around the processMeasures call in moveToProjectedPosition instead of directly within processMeasures avoids this issue.

I can investigate this further if it helps :slight_smile:

5

Thanks, I haven’t seen any issues yet in my testing, so if you can share a playground repro of the regression that’d be very helpful. Thank you!

6

Quick update - I was able to repro the layout issue with the fix and I’m continuing to investigate. Thanks!

1 Like
7

@lwp-joel - I tracked down the root of the layout issues the fix was causing, and have been able to fix that as well. In the repro I had, the original fix was interfering with textblock layout math. That issue is now addressed and all of our tests are passing.

Could you try this change with your scenario to confirm that it works correctly for you?

Thanks!

2 Likes
8

Yep, I can confirm it works! Thank you very much for the speedy fix!

2 Likes
9

You’re welcome, thanks for reporting this bug!

2 Likes