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

Add method to check if a physics server is enabled #90412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

caimantilla
Copy link

@caimantilla caimantilla commented Apr 9, 2024

It doesn't make much sense to be able to manually toggle the physics server, but not be able to check whether or not it's enabled.
My use case is that the game I'm working on has an editor tool which needs to enable the physics server to scan the level's collisions, which I then form a grid out of. The physics server is then disabled when the method finishes execution, but what if it was enabled before? Anything which relies on the physics server having been active is now broken. It'd be better to be able to restore the state rather than always disable it.
A similar case might appear in the future, where I'd only need the server to be active mid-frame when a mouse movement is detected to cast a ray to terrain, and then I can restore the server state once the ray has been cast.
The only caveat is that I think this would break compatibility with third-party physics engines like Jolt. It should only take a few lines to fix, though.

@caimantilla caimantilla requested review from a team as code owners April 9, 2024 03:50
@caimantilla caimantilla changed the title Add is_active() methods to physics servers Add method to check if a physics server is enabled Apr 9, 2024
@Zireael07
Copy link
Contributor

Would be nice to use it in some functions to prevent people being surprised that physics doesn't work in tool scripts

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Since set_active() and is_active() are both exposed, we can surely expose an active property now. This should be feasible while preserving compatibility. Setters/getters will disappear in the class reference in favor of the property, but that's OK.

@caimantilla
Copy link
Author

Since set_active() and is_active() are both exposed, we can surely expose an active property now. This should be feasible while preserving compatibility. Setters/getters will disappear in the class reference in favor of the property, but that's OK.

Ooh, good point. I'll update the PR soon with active implemented as a property.

@raulsntos
Copy link
Member

Since set_active() and is_active() are both exposed, we can surely expose an active property now. This should be feasible while preserving compatibility.

Note that this will break compat in C# until #90002 is merged.

@caimantilla caimantilla force-pushed the allow-check-physics-server-active branch from a8de9d3 to 8e1f8cc Compare April 25, 2024 08:27
@caimantilla caimantilla force-pushed the allow-check-physics-server-active branch from 8e1f8cc to 8fd516e Compare April 25, 2024 08:56
@caimantilla
Copy link
Author

Since set_active() and is_active() are both exposed, we can surely expose an active property now. This should be feasible while preserving compatibility. Setters/getters will disappear in the class reference in favor of the property, but that's OK.

Since set_active() and is_active() are both exposed, we can surely expose an active property now. This should be feasible while preserving compatibility.

Note that this will break compat in C# until #90002 is merged.

The C# PR has been merged and I believe I made the changes needed for active to be a property.
Am I missing anything?

@@ -760,6 +760,11 @@
<description>
</description>
</method>
<method name="_is_active" qualifiers="virtual const">
<return type="bool" />
<description>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<description>
<description>
Overridable version of [method PhysicsServer3D.is_active].

Copy link
Author

Choose a reason for hiding this comment

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

Is there some way that I can amend the commit with this change? The docs say that PRs should usually use just one commit...

Copy link
Member

Choose a reason for hiding this comment

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

You can just use git commit --amend and then push with git push -f

@Mickeon
Copy link
Contributor

Mickeon commented Jul 5, 2024

Poking to see if there is interest. It seems like the feature itself is fine, but it really cannot be merged currently. If you do not have the time or expertise to develop this further, feel free to say so and someone perhaps could take it from here.

@caimantilla
Copy link
Author

caimantilla commented Jul 5, 2024

Poking to see if there is interest. It seems like the feature itself is fine, but it really cannot be merged currently. If you do not have the time or expertise to develop this further, feel free to say so and someone perhaps could take it from here.

Ah, sorry, yeah, I've been busy. I've been meaning to update all my PRs eventually, just haven't felt much motivation to since 4.3 is in feature freeze anyways and I'm waiting for stable to update my game dev fork

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.

6 participants