Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Physics Interpolation - add helper warnings #60531

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Apr 26, 2022

When physics interpolation is active on a node, it is essential that transforms are updated during "_physics_process()" rather than "_process()" calls, for the interpolation to give the correct result.

This PR adds optional warnings for instances, cameras and multimeshes which can flag updates being incorrectly called, and thus make these problems much easier to fix.

The warnings only occur in DEBUG_ENABLED builds, and when physics interpolation is enabled, and can be switched on and off with project setting debug/settings/physics_interpolation/enable_warnings.

For instances, it prints the node name in the warning. For cameras and multimeshes there is no name available, this is unlikely to be a problem with cameras (as there is normally only 1 or 2) and the warning for multimesh is better than none at all. There is no objectID stored per multimesh currently. It could be made to store this, but is probably overkill for this warning feature.

When the warning is issued, the usual step to take is to examine the object scripts etc and move anything being updated in _process() to _physics_process(). Additionally some "magic" nodes (tweening etc) may move nodes automatically, these may similarly need to have their mode flipped to operate on physics tick rather than idle.

The warning uses the wording "usually", and "possible benign", because it is not a problem to occasionally move nodes about in e.g. _ready() or teleported as a result of an event in _process(). But usually when the warnings are showing a lot it will indicate a problem.

Example output

W 0:00:30.384 instance_set_transform: Interpolated Instance "SunCore" transform should usually be set during physics process (possibly benign).
W 0:00:32.788 instance_set_transform: Interpolated Instance "ChargeShockwave" transform should usually be set during physics process (possibly benign).
W 0:00:36.990 camera_set_transform: Interpolated Camera transform should usually be set during physics process (possibly benign).

Notes

  • There are no changes to functionality here, just additions of some optional warnings.
  • The warnings only occur in DEBUG builds (i.e. mainly editor, and not the release templates).
  • While testing @RPicster 's beat invaders game it became apparent it would be super helpful for users to be able to pinpoint where they are updating objects incorrectly, especially when converting existing projects, or for those new to physics interpolation.
  • Warnings aren't excessively spammed, they are only sent every 2048 or so tries (usually every few seconds).
  • The checking of the project_settings value are only done infrequently, so I'm not overly worried that these will create a performance issue at runtime. The alternative would be to cache the setting.

When physics interpolation is active on a node, it is essential that transforms are updated during "_physics_process()" rather than "_process()" calls, for the interpolation to give the correct result.

This PR adds optional warnings for instances, cameras and multimeshes which can flag updates being incorrectly called, and thus make these problems much easier to fix.
@lawnjelly lawnjelly force-pushed the fti_usage_warnings branch from ba66967 to ad9b2b3 Compare April 26, 2022 12:56
@@ -33,6 +33,10 @@
#include "core/os/os.h"
#include "core/print_string.h"

#if defined(DEBUG_ENABLED) && defined(TOOLS_ENABLED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOOLS_ENABLED assumes DEBUG_ENABLED so you don't need to check the latter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said if you prefer to have both to make the intent extremely clear, that's fine too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I remember this coming up before. I'm slightly in favour of keeping both as you say to make the intent clear, because to a reader it is not obvious that TOOLS_ENABLED implies DEBUG_ENABLED (and this could conceivably change in future).

@akien-mga akien-mga merged commit 4cfc96f into godotengine:3.x Apr 28, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants