fix(ND): Plan Mode waypoint jumping

[vkrizan]

Fixes #7421

Summary of Changes

This tries to fix ND render in Plan Mode, when vectors and symbols are jumping around when selecting different waypoints (scrolling).

A cause of this is that the two position simvars (A32NX_SELECTED_WAYPOINT_LAT and A32NX_SELECTED_WAYPOINT_LONG) get values from useSimVar independently.

mapParams in PlanMode are now fully statefull. A new MapParameters is created for new computation, which kicks in re-render. Version is increased. The mapParamsVersion} assignment within PlanMode is no longer needed.

The (general) problem of mapParams is that only one instance was kept and recomputed, which needed children to re-render on different prop changes.

Note that MapParameters objects are relatively small and should not impact GC.

Screenshots (if necessary)

References

Additional context

Discord username (if different from GitHub):

Testing instructions

1. Load some flight plan with many waypoints.
2. Switch ND to PLAN.
3. Switch MCDU to F-PLAN.
4. Scroll through waypoints.
5. No vector jumping should be observed when scrolling on MCDU page (expect smoother transitions).
6. No plane icon jumping should be observed when scrolling on MCDU page (especially on waypoints close to the plane).
7. Fly the plane and do a regression testing on PLAN, ARC and ROSE ND modes.

Note, that the waypoints might blink when selecting waypoints when on PLAN ND mode. I suspect that this is because they are added to the SVG when within the viewport and remove when they are outside.

How to download the PR for QA

Every new commit to this PR will cause a new A32NX artifact to be created, built, and uploaded.

1. Make sure you are signed in to GitHub
2. Click on the Checks tab on the PR
3. On the left side, click on the bottom PR tab
4. Click on the A32NX download link at the bottom of the page

[2hwk]

There are some concerns if there may be a noticeable performance regression because of this change, since any increase in GC load, minor or not, usually has a noticable impact on performance. Although it is not usual to be concerned about performance at such a low level, we ultimately have no choice here but to take this into account.

There's also a change of design from moving from a single instance of a mapParams object, and there should be a way of fixing the issue with a less significant modification.

Additionally, we are already in the process of rebuilding the ND using the MSFS avionics framework, where this issue has already been fixed.

Note: This issue was first created when the new react useSimVar hook was merged into master, at the time this was noticed but the trade-off to increase performance was an overriding consideration.

[vkrizan]

There are several problems of having a single instance of MapParams, mainly the state management. Components are re-rendered (recomputed) when there is a change of an object instance (not its values). Use of mapParametersVersion is very suboptimal and rigid. The state change is not propagated downwards correctly. This issue then does not allow changes of the map params within callbacks (as the debounce one).

Memory and GC impact of creating a new MapParams objects is limit to zero. It it a struct with couple of integers/floats that are created nevertheless. Having this as a single instance seems like a premature optimization. I would be more worried more about re-renders, as those impact rendering and create much more memory to be GC'd than a simple struct.

The the new react useSimVar hook actually uncovers the problem of bad state management.

The rebuild of the ND to MSFS avionics sound very promising (seems like it is within the nd-v2 branch). That one should give more controls over the renders (as advertised by the lib). The principle of the creating a new objects for state management could still apply for the rewrite. Otherwise there a risk of bad state.

@2hwk Please let me know how to proceed from here. Should I spend more time on this and resolve the re-renders with potential GC increase? What simpler solution do you suggest? Technically if those two simvars can be synchronized, then that could be a quick fix w/o many changes.

Thanks.

[2hwk]

There are several problems of having a single instance of MapParams, mainly the state management. Components are re-rendered (recomputed) when there is a change of an object instance (not its values). Use of mapParametersVersion is very suboptimal and rigid. The state change is not propagated downwards correctly. This issue then does not allow changes of the map params within callbacks (as the debounce one).

Memory and GC impact of creating a new MapParams objects is limit to zero. It it a struct with couple of integers/floats that are created nevertheless. Having this as a single instance seems like a premature optimization. I would be more worried more about re-renders, as those impact rendering and create much more memory to be GC'd than a simple struct.

The the new react useSimVar hook actually uncovers the problem of bad state management.

The rebuild of the ND to MSFS avionics sound very promising (seems like it is within the nd-v2 branch). That one should give more controls over the renders (as advertised by the lib). The principle of the creating a new objects for state management could still apply for the rewrite. Otherwise there a risk of bad state.

@2hwk Please let me know how to proceed from here. Should I spend more time on this and resolve the re-renders with potential GC increase? What simpler solution do you suggest? Technically if those two simvars can be synchronized, then that could be a quick fix w/o many changes.

Thanks.

You are right that the state management right now is somewhat flawed and that ultimately this should be addressed, I am thinking perhaps this PR could instead be re-targeted at the NDv2 branch (https://github.com/flybywiresim/a32nx/tree/nd-v2)?

[vkrizan]

@2hwk I'm not sure at what stage is the NDv2. I guess I can give it a try. Alternatively I can try to check the impact of this PR on re-renders and maybe you folks can decide to have this as a preliminary fix.

[vkrizan]

The NDv2 does not have this problem. Transition between waypoints is smooth. Waypoints are a bit blinking (as are with this PR). I guess it is because they are injected into SVG when in viewport.

[2hwk]

Moving back to draft (close if unnecessary) for now to streamline the QA project. Set as ready to review if there's any follow up.

[Benjozork]

Superseded by #7908

We appreciate the work though.