Immediate Ambient Light Updating To Prevent Flicker On Transition To …

[AverniteDF]

…Exterior

The purpose of this PR is to eliminate the glitchy flicker you sometimes get when exiting a building.

What the world outside looks like for a few frames before lighting gets updated:

And after:

Immediate calls to PlayerAmbientLight.UpdateAmbientLight solves this.

The proposed code calls that method twice: once in the current frame and again on the next. The combination of the two calls completely eliminates all flickering as far as I can tell (I disabled HUD fade in so I could see every frame).

[petchema]

I think Event/Delegate would be better style here (less coupling).

[petchema]

I did not notice the 3Hz polling here, that explains some delayed updates...
I wonder if it would be possible to use other mechanisms (events/degates?) to get rid of polling (and its delays vs performance tradeoffs) without introducing coupling, are there best practices in Unity/C# in this domain?

[petchema]

To ensure that "mutex" changes are immediately seen by all threads, I think it should at very least be declared volatile.
Busy looping is also terrible(1) and doesn't work (it's immediately followed by a race). If you don't want to go the full Mutex class way, use at least a lock() statement

1. If fact if mutex ever happens to be true at the moment while (mutex) ; is executed, the thread is likely to hang forever: https://learn.microsoft.com/en-us/archive/msdn-magazine/2012/december/csharp-the-csharp-memory-model-in-theory-and-practice "Broken Polling Loop"

[AverniteDF]

The code given is primitive, yes. I'm not super familiar with C# yet or multithreading. It doesn't look like there's anything too expensive in the ambient light update logic. Could it simply be called every game tick instead of being run as a coroutine?

…

On Tue, Nov 1, 2022 at 6:39 AM Pierre Etchemaïté ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Assets/Scripts/Game/PlayerAmbientLight.cs <#2454 (comment)> : > @@ -52,34 +56,49 @@ void Update() StartCoroutine(ChangeAmbientLight()); } + bool mutex; To ensure that "mutex" changes are immediately seen by all threads, I think it should at very least be declared volatile. Busy looping is also terrible. If you don't want to go the full Mutex class way, use at least a lock() statement — Reply to this email directly, view it on GitHub <#2454 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3QEP2R7L3TS63JBNXMZQELWGDXPPANCNFSM6AAAAAARTVBGEY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[AverniteDF]

I've put up a cleaner implementation (no mutex stuff and ManageAmbientLight coroutine has been removed). In current form it still requires the two calls to PlayerAmbientLight.UpdateAmbientLight from other classes, otherwise you get two frames of incorrect lighting after interior->exterior transition.

It is helpful to set FadeBehaviour.allowFade to false when testing this so you can more clearly see what's going on during transition.

[AverniteDF]

I think it would be best to call UpdateAmbientLight() in the main thread, as late as possible and immediately before the drawing of each frame.

Are there any suggestions as to where that might be? I tried doing it in LateUpdate() but that didn't seem late enough.

The other option as I see it is to call UpdateAmbientLight() on demand as needed, in which case there'd be no need to call it every frame or periodically.

With the second option, UpdateAmbientLight() would be called after, and whenever, player location or game time is changed.

[petchema]

The other option as I see it is to call UpdateAmbientLight() on demand as needed, in which case there'd be no need to call it every frame or periodically.

With the second option, UpdateAmbientLight() would be called after, and whenever, player location or game time is changed.

Yes, that's what events/delegates could be used for. It's a publication/subscription mechanism, so code notifying about changes (location, weather,...) would unaware of the PlayerAmbientLight object (and possibly other objects) listening for those notifications; This decreases coupling and is more dynamic than plain method calls.

[AverniteDF]

There is an existing event that gets fired at the end of the transition to exterior logic called RaiseOnTransitionExteriorEvent() that may be of note. I've further simplified the PR code. Calling UpdateAmbientLight() every tick in LateUpdate() eliminates the need to call it from SaveLoadManager.RestoreCachedSceneNextFrame(), so that class no longer needs to be modified, and now there is just the one call from PlayerEnterExit.BuildingTransitionExteriorLogic() which could probably be moved into the event like you suggested.

…

On Wed, Nov 2, 2022 at 11:11 AM Pierre Etchemaïté ***@***.***> wrote: The other option as I see it is to call UpdateAmbientLight() on demand as needed, in which case there'd be no need to call it every frame or periodically. With the second option, UpdateAmbientLight() would be called after, and whenever, player location or game time is changed. Yes, that's what events/delegates could be used for. It's a publication/subscription mechanism, so code notifying about changes (location, weather,...) would unaware of the PlayerAmbientLight object (and possibly other objects) listening for those notifications; This decreases coupling vs plain method calls. — Reply to this email directly, view it on GitHub <#2454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3QEP2U3KAUOUE4VNHN5EZDWGKAC7ANCNFSM6AAAAAARTVBGEY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[AverniteDF]

Ah, that does work better. The event handler got rid of the need to modify PlayerEnterExit and also the Instance member I added to PlayerAmbientLight.

[AverniteDF]

There's a wrinkle inPlayerEnterExit.BuildingTransitionExteriorLogic()that I'd like to point out.

It has to do with the last two lines of code in that method:

// Update serializable state from scene cache for interior->exterior transition
SaveLoadManager.RestoreCachedScene(world.SceneName);

// Fire event
RaiseOnTransitionExteriorEvent();

The call toRaiseOnTransitionExteriorEvent()is not quite accurate here because the previous line,SaveLoadManager.RestoreCachedScene(), attempts to restore the cached scene on a separate thread.

So other classes are being notified that the transition has completed when in fact it is still pending.

I believe this is the reason why I kept getting a pesky ambient lighting flicker when exiting buildings, and I originally needed a second call toUpdateAmbientLighting()at the bottom ofRestoreCachedSceneNextFrame().

TheRaiseOnTransitionExteriorEvent()call could be passed toRestoreCachedScene()as a post-Action and this would fire the event upon actual transition completion. However, downstream code in the calling thread may be relying on the event being fired as currently implemented.

It would be nice to eliminate theRestoreCachedSceneNextFrame()method, if possible. Find out what needs to be registered and do the scene restoration immediately instead of on a separate thread.

[AverniteDF]

Okay I found what was causing the pesky flicker. It wasn't the delayed cached scene restoration. It was happening because SunlightManager wasn't in a ready state when CalcDaytimeAmbientLight() was being called.

It seems that SunlightManager.Upate() isn't called per tick when the player is indoors, or at least not reliably. I know that if you load a character who is inside a building the first time you exit the data members of SunlightManager aren't correct until its Update() method gets called.

It's working quite well now.

[Interkarma]

Thank you for the PR. I know the flicker you're looking at fixing, and appreciate the suggested fix. I also appreciate the refinement after @petchema's review. Cheers both. :)

[KABoissonneault]

Is this still relevant? If the original contributor is still around, I'd like for them to revert the changes PlayerEnterExit.cs and SaveLoadManager.cs, since it's just whitespace. Otherwise, I can give this a run and see if I notice a difference

[petchema]

The original issue should still be around, easiest way to spot it is probably to exit a tavern at night, and notice the exterior is bright for a split second before light abruptly adjusts to dark then ramps up to final correct exterior lighting. Effect varies a bit between tries, depending on when the 3hz light adjustment polling takes place vs. your building exit, but it's there.
With this patch when you exit that same tavern the light is immediately dark and ramps up to final correct exterior lighting, no initial flash.
(Whether it's the best way to solve that issue is another story, cleanly handling transient states is hard.)