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

Curve: Prevent forcing 1.0 min value to 0.99 #29959

Merged
merged 1 commit into from
Jun 21, 2019

Conversation

akien-mga
Copy link
Member

The setters are called when the property is first initialized, and before
that its default min and max are 0.0 and 1.0 respectively.

If you configured min_value to 1.0 and max_value to e.g. 3.0, since the
min_value setter can be called before that of max_value (which thus still
defaults to 1.0), the min will be set to 0.99.
Same conflict could happen with a configured max_value of 0 if its setter
is called before that of a valid, negative min value.


Not sure if it's the cleanest way to do a tri-state "boolean", if anyone has a better proposal, go ahead :)

The setters are called when the property is first initialized, and before
that its default min and max are 0.0 and 1.0 respectively.

If you configured min_value to 1.0 and max_value to e.g. 3.0, since the
min_value setter can be called before that of max_value (which thus still
defaults to 1.0), the min will be set to 0.99.
Same conflict could happen with a configured max_value of 0 if its setter
is called before that of a valid, negative min value.
@akien-mga akien-mga added this to the 3.2 milestone Jun 21, 2019
@akien-mga akien-mga requested a review from Zylann June 21, 2019 14:09
@akien-mga akien-mga merged commit 10cf5ac into godotengine:master Jun 21, 2019
@akien-mga akien-mga deleted the dont-reset-my-curves branch June 21, 2019 19:17
@Zylann
Copy link
Contributor

Zylann commented Jun 23, 2019

About the fix itself: just use two bools, or use a uint8_t, because an int will be heavier than these :p

More general comment:
Although it fixes the issue, it's a long-lasting problem I keep worrying about in Godot's serialization system in general: when a property's value depends on another to be validated or compute something, you can no longer automatically serialize the object by randomly iterating over its properties. Failure to comply then causes bugs or ineficient construction of the object.
This is avoided by luck, goodenough or hips&hoops in the engine so far, but I had the issue several times even when writing plugins so already had to figure out workarounds.

A few workarounds:

  • Hack it, like you did. Works but makes the code a bit confusing.
  • Don't validate on set, but we have to because we have mixed inspection and serialization
  • Don't validate at all: this might mathematically work in some cases but could be confusing
  • Have a way to properly know when the object has fully been auto-serialized in order to post-validate. Or, have a way to pre-validate the data, or to customize serialization entirely with a function. But once again I'm not sure if we want that in the engine. First solution ends up doing that manually.
  • Pack all data inside a _data dictionary which is then serialized with _get and _set with only USAGE_STORAGE, leaving min and max with only EDITOR_STORAGE. This is actually the cleanest way right now because it allows to validate min and max simultaneously. It's a bit tedious to write, but I used that a few times in a plugin. Only functional downside is that variables are inherently saved and loaded simulateously, i.e you can't have an override of only max in some inherited scene for example, although here it's about a resource.

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