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

Fix hints on PhysicsMaterial bounce/friction. #42203

Merged
merged 1 commit into from
Sep 23, 2020

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Sep 19, 2020

These values are only meaningful in the range 0 to 1.
Make sure the editor enforces reasonable values.
Fixes #42202.

These values are only meaningful in the range 0 to 1.
Make sure the editor enforces reasonable values.
Fixes godotengine#42202.
@Calinou Calinou added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:editor topic:physics labels Sep 19, 2020
@Calinou Calinou added this to the 4.0 milestone Sep 19, 2020
@akien-mga
Copy link
Member

Are you sure that the values outside 0..1 can't be used for non-physically correct behaviours? Like bounce > 1 to increase bounce height each time (breaks energy conservation, but we're making games :)).

@rcorre
Copy link
Contributor Author

rcorre commented Sep 20, 2020

Interesting, I'll have to try that 😆
FWIW I stole the hints from PhysicalBone. Whatever the contraints are (even if there are none), should those two be in sync?

@rcorre
Copy link
Contributor Author

rcorre commented Sep 20, 2020

Unfortunately values greater than zero don't seem to make a difference (at least in Godot 3.2 with Bullet Physics).
Peek 2020-09-20 09-23

ADD_PROPERTY(PropertyInfo(Variant::BOOL, "rough"), "set_rough", "is_rough");
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "bounce"), "set_bounce", "get_bounce");
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "bounce", PROPERTY_HINT_RANGE, "0,1,0.01"), "set_bounce", "get_bounce");
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use or_greater hint while still restricting the slider to 0..1 range, which should be common for most use cases anyway, but still allow for specific use cases (for values greater than >1, can be manually typed via keyboard), see #36450 for instance.

Suggested change
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "bounce", PROPERTY_HINT_RANGE, "0,1,0.01"), "set_bounce", "get_bounce");
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "bounce", PROPERTY_HINT_RANGE, "0.0,1.0,0.01,or_greater"), "set_bounce", "get_bounce");

Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, the new behavior in master is that you can still drag the slider past the max value, but the slider will be snapped to the specified range instead, this is not the case for Godot 3.2, IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know! Given that > 1 didn't seem to be meaningful here after all, I think hard limits make sense.

Copy link
Contributor

@Xrayez Xrayez Sep 23, 2020

Choose a reason for hiding this comment

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

Hmm I guess I just happen to have some physics bodies in 2D with custom physics, it's possible to set values above >1 just like Akien mentioned, so in theory the physics material could be used for non-engine classes, since it's agnostic to the physics backend used. It's a small nuance, but that's something which I'd do myself, but the engine development philosophy is that everything must be kept simple to cater the most common use cases, so I can only comply. 😛

The physics material itself probably just clamps the value to the range of 0..1 anyway.

Copy link
Contributor

@Xrayez Xrayez Sep 23, 2020

Choose a reason for hiding this comment

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

No, it doesn't 🎉:

void PhysicsMaterial::set_bounce(real_t p_val) {
bounce = p_val;
emit_changed();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Maybe or_greater would be appropriate here then. I'll leave that to you to PR if you think its useful though :)

Copy link
Member

Choose a reason for hiding this comment

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

The physics engines do clamp the calculated bounce to 0..1:

body_pair_2d_sw.cpp
214:real_t combine_bounce(Body2DSW *A, Body2DSW *B) {
215:    return CLAMP(A->get_bounce() + B->get_bounce(), 0, 1);

(at least in 2D)

Sure you can set a negative bounce on one body of -5.0 and the other one +5.5 and they should amount to +0.5 and be within bounds... But why? We don't cater to undefined use cases, and if there's no implemented physics engine that actually supports values outside 0..1 (as in, it's designed in a way where those values actually make sense, not that they can incidentally produce an outcome which is not outright a bug), there's no reason to future proof.

If you have a very non-standard use case, you can use a script to set those values to arbitrary values. Or create a UnlimitedPhysicsMaterial resource in a script :)

We can discuss this in https://github.com/godotengine/godot-proposals, but I don't see the point in allowing values that don't make sense currently in the engine.

Choose a reason for hiding this comment

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

Thanks, I'm changing CLAMP to 0, 10 in my fork...

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned, if you have a use case for unclamped bounce or friction, please open a proposal on https://github.com/godotengine/godot-proposals

This PR did not restrict anything further than what the codebase already does - even if you could input 10 before this change in the inspector, the engine would clamp it to 1 in body_pair_2d_sw.cpp as I've demonstrated above.

@akien-mga akien-mga merged commit cadba26 into godotengine:master Sep 23, 2020
@akien-mga
Copy link
Member

Thanks!

@rcorre rcorre deleted the physicsmat_hint branch September 23, 2020 12:23
@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 24, 2020
@TetraGenius
Copy link

Various physics engines allow the user to use bounce > 1. Maybe a paddle or block that increases speed and difficulty. I use it to simulate gas particles hitting a hot wall (increases their speed/pressure). But if someone can point me to making a UnlimitedPhysicsMaterial resource I'll try that...

@TetraGenius
Copy link

You can also check superelastic restitution in research.

@TetraGenius
Copy link

Thanks, I'm changing CLAMP to 0, 10 in my 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.

PhysicsMaterial field exports should be bounded
5 participants