[3.x] Physics Interpolation - Fix global switch hidden nodes

[lawnjelly]

Ensure hidden nodes get reset when switching global physics interpolation on / off.

Fixes #101192 for hidden nodes.

Notes

• It had been a while since I looked at it, and it turns out for 2D and 3D transforms are currently kept up to date for hidden items, so the general case unhiding already seems to work fine. (I had already added a special case for 3D with physics interpolation, although this whole area may be revisited.)
• This means for now, fix for this issue can purely be for addressing the rare case of switching on / off global physics interpolation at runtime.
• Localized to SceneTree so no changes are needed within nodes.
• Also applicable to 4.x.

Discussion

Another alternative (as @akien-mga suggested) is just to unexpose the function which allows changing physics interpolation at runtime, and withdraw support, and get users to restart the engine with a cfg file.

That alternative is available in #102762 .

Similar problems could also theoretically occur when changing branches interpolation mode at runtime, but this is intended to be design time decision (I'll probably look at adding this to the docs), I'm not sure there is a use case for this at runtime.

It is also possible to put logic in the nodes themselves just for this settings change (e.g. checking a global state before checking visibility), but imo it would be better to avoid complicating the code and maintenance for such a rare use case (we only have one user asking for the feature).

[kiroxas]

I do not know how nodes and physics works internally, so feel free to ignore if this makes little sense. However this seems like it could have complications if some node have some costly code in response to when setVisible is called. Seems like you are targeting some specific nodes, which I assume does not have this property right now, but this use case ( toggle on then off in the same frame) is not something people will think of when adding new features, so that could easily bite them. Is there really no way to add a parameter to reset_physics_interpolation() so it consider the hidden nodes too? That would seem to be the way with less collateral damage.

[lawnjelly]

However this seems like it could have complications if some node have some costly code in response to when setVisible is called.

Side effects is always a concern, but this is very rarely called function (like maybe called once if at all, it's effectively a global on / off switch for game settings menu), so it can be hideously inefficient and there is no problem. So in this case simplicity and code maintenance is a more important concern than efficiency.

I did think of putting a note on this (visibility enablers, user scripts responding to visibility changes), but if there is evidence of side effects in the wild, there are plenty of other approaches that are available. I do subscribe to KISS principle unless there is evidence of a need for more engineering here, I feel going further for such a rare use case at this stage is over-engineering.

There are several reasons to prefer not calling reset on hidden nodes:

• If it can be avoided - don't spread complexity to the nodes themselves for a rare use case. There's no need to add this to new nodes, it should just work. Localize solutions wherever possible.
• There is no guarantee that hidden nodes will have up to date transforms, this is not warranted.
• Localizing the solution is future proof in case we decide to change the unhiding behaviour.

See https://docs.godotengine.org/en/stable/contributing/development/best_practices_for_engine_contributors.html .

Seems like you are targeting some specific nodes, which I assume does not have this property right now, but this use case ( toggle on then off in the same frame) is not something people will think of when adding new features, so that could easily bite them.

I'm not sure I understand what you are saying here. If anything, it's the opposite, see above.

(Toggling node visibilties within the frame many times (in any combination) is guaranteed to be supported, as users can do this in their own scripts. It is by definition safe, although not guaranteed to be efficient. )

[KeyboardDanni]

I'm inclined to agree with @kiroxas. This behavior introduces side-effects that could cause problems for anything that handles visibility notifications. How would you be able to differentiate between a visibility change done as a result of a settings change? All visibility change logic would need knowledge of a seemingly unrelated system to avoid issues.

I did think of putting a note on this (visibility enablers, user scripts responding to visibility changes), but if there is evidence of side effects in the wild, there are plenty of other approaches that are available. I do subscribe to KISS principle unless there is evidence of a need for more engineering here, I feel going further for such a rare use case at this stage is over-engineering.

IMO users shouldn't have to read the docs to know about implementation side-effects.

It is also possible to put logic in the nodes themselves just for this settings change (e.g. checking a global state before checking visibility), but imo it would be better to avoid complicating the code and maintenance for such a rare use case (we only have one user asking for the feature).

As the one user asking for this feature, I am willing to take on that code and maintenance burden :)

[lawnjelly]

Actually to some extent this PR is outdated now as we are moving to scene side interpolation. In fact I'm pretty sure I already fixed this up (or one aspect of it) in the new code.

So I'll close and we can revisit after that is done.