CST-306: update edge cases of rotation, target, and fov updates

[pcuste1]

What is changing?

update edge cases of rotation, target, and fov updates

Why is this change needed?

Reduced multi updates

When aladin-lite is updating the target or fov, it plays an animation that triggers a callback event at intermediate target/fov values between the starting and destination values. If a user listens for these callback events, they end up with a very jittery sequence of events, eventually winding up at the correct target/fov. By limiting the updates to the ipyaladin _target and _fov parameters in event_handler.json to only the final event in that chain, we are able to eliminate virtually all of the jitter, and the user only gets a callback on the final destination of the target/fov.

Before:

aladin-lite-pre-dedupe-fix-480p.mov

After:

aladin-lite-post-dedupe-fix-480p.mov

Rotation updates

while testing this code, it was identified that when the rotation is updated from the API, it can get into a weird edge case where the _fov_xy doesn't get reset before the rotation gets set, which causes the whole thing to error out. By removing the zeroing out of _fov_xy we don't run into this issue anymore. It wasn't playing a part in how rotation or fov_xy was being updated, so this shouldn't be breaking any other workflows.

It was also identified that a call to updateRotation was being called after the rotation was already updated by the python code. This effectively meant that widget.js would update rotation, and trigger aladin-lite to update rotation. aladin-lite would then trigger a change:_rotation event which would update rotation again, but with an outdated value.

Before:

After

[cpparts]

Going to check out the branch tomorrow to dig in. Would love to have some of the example problem workflows to play with to understand the updates!

[cpparts]

I can't seem to duplicate the screenshots you've shown. Are you "run all"-ing on these cells?
This is the only call that updates rotation in this event_handler and was implemented to parallel what was defined for "change:_height". Is the implementation for height then also an issue for jitter? Do we need to rework both? Remove updateRotation?

[pcuste1]

yup, I'm positive that this is causing an error. The rotation doesn't need to be updated from the event handler since its already updated through the python code. This would need to be here is there was a UI method for updating the rotation, but then we'd need to implement a locking mechanism similar to fov or target to avoid this issue

[tomdonaldson]

The event handling here is quite hard to follow. These locks seem to basically cause the next event of another kind to be ignored, but it's not obvious what actions/functions trigger those events. Still trying to wrap my head around it.

[cpparts]

Why we can remove this without causing issues/it being poor practice? The updated aida fov definition relies on fov_xy, so I am a bit confused. Can you share a sample workflow of running into this issue? Also, what do you mean by "reset" in reference to fov_xy and rotation?

[pcuste1]

I can share an example once I get all my of PRs related to this going. But basically setting these to {} is effectively resetting them to null, which was causing issues for me. I'm not sure why this is even included in any of these methods initially since the javascript code handles firing all of the events to trigger updates on these.

What is currently implemented follows this workflow

1. set _fov_xy and _wcs to {}, which is effectively null
2. set _rotation to the user defined rotation value
3. use javascript method to trigger an update to the rotation in aladin-lite
4. aladin-lite updates the rotation, no callback is triggered
5. the update to _rotation triggers the change:_rotation callback which updates the wcs and fov_xy

Step 1 of that workflow doesn't actually have any impact on the success of that workflow as best I can tell, but can lead to some race conditions if the rotation is updated, then a listener on rotation triggers a method which relies on wcs and/or fov_xy before step 5 is ran.

since we are only able to update the rotation through the api right now, I believe we can assume that wcs and fov_xy won't change as the result of someone calling this method, so setting them to {} shouldn't be necessary

[tomdonaldson]

Can all this be run with just this version of ipyaladin or are there dependencies on other modules? I agree that it would be helpful to be able to run your examples.

[pcuste1]

Scenario:
We set an observe on the _rotation traitlet with a simple function that prints out the fov_xy value. When we update aladin.rotation via. the api, the function will be ran as soon as the traitlets value is updated in aladin.

Before fix:
Description: When the final cell is ran, the rotation is updated in ipyaladin, _fov_xy is set to {} and then the callback_function is unable to access the fov_xy value

After fix:
Description: When the final cell is ran, the callback_function has no issue access the _fov_xy value, which should not change as a result of updating the rotation

Relevant error text:

Cell In[3], line 2, in callback_function(caller)
      1 def callback_function(caller):
----> 2     print(aladin.fov_xy)

File [~/Workspace/ipyaladin/src/ipyaladin/widget.py:394](http://localhost:8888/lab/workspaces/auto-O/tree/notebooks/~/Workspace/ipyaladin/src/ipyaladin/widget.py#line=393), in Aladin.fov_xy(self)
    385 """The field of view of the Aladin Lite along the two axes.
    386 
    387 Returns
   (...)
    391 
    392 """
    393 if self._fov_xy == {}:
--> 394     raise WidgetNotReadyError(
    395         "The field of view along the two axes is not available. This often "
    396         "happens when the FoV is modified and read in the same cell. "
    397         "Please read it from a new cell."
    398     )
    399 return (
    400     Angle(self._fov_xy["x"], unit="deg"),
    401     Angle(self._fov_xy["y"], unit="deg"),
    402 )

WidgetNotReadyError: The field of view along the two axes is not available. This often happens when the FoV is modified and read in the same cell. Please read it from a new cell.

[pcuste1]

I found a simplified observe method that illustrates the issue! It's a race condition between when the _rotation value is updated in the python code and when the _fov_xy is updated by the javascript code. Since we are relying on the observe method for real time syncing of viewers, this needs to be accounted for

[tomdonaldson]

I'm comfortable with these changes as written.

My discomfort is more general in that the event propagation and locks seem fragile to me, so I'm worried we'll inadvertently violate a hidden assumption. We can learn more during PR discussion if/when these changes are proposed upstream.

[cpparts]

I just merged in the changes from the upstream dev (ipyaladin/dev) into dev_cobalt. This includes updates to the way in which rotation is set, which I am hopeful will clean up some of the jitter and reduce any changes you might need to make in this PR. I understand it's inconvenient to pull these changes back into your work and see what's changed, but I would love to get your thoughts on any behavior differences you might see! Hopeful this will address some of the buggy behavior you've tracked down!

[pcuste1]

The upstream changes do resolve the issues that this PR was also trying to resolve! Everything is working well using the newer changes, so I'll close out this PR. Unfortunately its not addressing the jitter at all, but that's mostly on the imviz side, so I'm not concerend