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

Improve process logic in Camera2D #46717

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Mar 6, 2021

The logic for internal process and internal physics process in Camera2D was very buggy and convoluted for historical reasons.

This is a cleanup to make the logic simpler and easier to follow.

Notes

  • Moved this out of draft to try and get some eyes on..
  • I've added an active flag rather than complicated the logic with lots of is_editor_hint spread all over the place. The active flag is set in one place, and makes it far easier to read / should make it less bug prone.
  • This deprecates the ability to turn off processing by setting follow_smoothing to zero, and the old _set_old_smoothing function.

@Chaosus Chaosus added this to the 3.2 milestone Mar 7, 2021
@lawnjelly lawnjelly marked this pull request as ready for review March 8, 2021 07:44
@lawnjelly lawnjelly requested a review from a team as a code owner March 8, 2021 07:44
scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
@lawnjelly lawnjelly force-pushed the camera_process branch 2 times, most recently from 69346d8 to c2067ed Compare March 8, 2021 11:12
@lawnjelly
Copy link
Member Author

While testing I noticed a slight vulnerability. The user could manually set the camera process mode using node functions, and bypass the camera logic. So I've added an extra check for this (_is_process_mode_correct called in the transform notification), it should be pretty cheap for an extra layer of protection. It also gives some insulation against further logic bugs in future.

We could place a WARN_PRINT_ONCE when this occurs? Wasn't sure whether this would be preferred.

scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

The current code changes seem sensible to me. I'll try to test some projects to make sure that things behave as expected.

While testing I noticed a slight vulnerability. The user could manually set the camera process mode using node functions, and bypass the camera logic. So I've added an extra check for this (_is_process_mode_correct called in the transform notification), it should be pretty cheap for an extra layer of protection. It also gives some insulation against further logic bugs in future.

We could place a WARN_PRINT_ONCE when this occurs? Wasn't sure whether this would be preferred.

I think that's a general design issue which extends beyond just the Camera2D API. The internal process modes were added specifically to handle Node-internal logic that shouldn't be messed with by users. Yet, we still expose those methods to let them shoot themselves in the foot, just like they can queue_free() child nodes which are part of the private API of more complex nodes.
IMO it's OK as long as documentation is clear about it ("With great power comes great responsibility"), but some sanity checks here and there shouldn't hurt either.

@akien-mga
Copy link
Member

akien-mga commented Mar 8, 2021

IMO it's OK as long as documentation is clear about it ("With great power comes great responsibility"), but some sanity checks here and there shouldn't hurt either.

Documentation seems to be alright:

Warning: Built-in Nodes rely on the internal processing for their own logic, so changing this value from your code may lead to unexpected behavior. Script access to this internal logic is provided for specific advanced uses, but is unsafe and not supported.

The logic for internal process and internal physics process in Camera2D was very buggy and convoluted for historical reasons.

This is a cleanup to make the logic simpler and easier to follow.
@akien-mga akien-mga merged commit 3fafaf2 into godotengine:3.2 Mar 8, 2021
@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.

3 participants