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 some invalid int property ranges #89566

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 16, 2024

  • HeightMapShape3D had ranges configured for float instead of int
  • Particles had amount that used exp which is not supported, added a note

The amount case could be fixed by adding support for this to EditorPropertyInteger but unsure how useful that would be generally, so just fixing it here

@AThousandShips AThousandShips added this to the 4.3 milestone Mar 16, 2024
@AThousandShips AThousandShips requested review from a team as code owners March 16, 2024 11:06
@Mickeon
Copy link
Contributor

Mickeon commented Mar 16, 2024

I think it would be useful. Exp was probably used in an attempt to have more control over the lower values (where particle count is WAY more noticeable).

@AThousandShips
Copy link
Member Author

We can always add it back in if we improve that side, but ensuring it works correctly might be finicky, and might be buggy with integer values especially in the lower range (though I haven't tested it myself)

So opening a separate PR and adding different cases that would benefit from exp (including these ones back in) seems to be the way to go with that side

@KoBeWi
Copy link
Member

KoBeWi commented Mar 16, 2024

Instead of removing it, which does not change anything, you could put a FIXME comment.

@AThousandShips
Copy link
Member Author

Sure can do that instead, this does do one thing though: Discourage others adding this to their code thinking it will work

@AThousandShips
Copy link
Member Author

With just a quick check, without doing any deeper reworking, adding support for exp to this specific property makes it impossible to edit reliably, it only affects the grab behavior, and skips wildly, it only works reasonably well with the slider exposed, which won't happen without a step smaller than 1

So I think the better solution is simply to remove it, I don't see the usefulness of it, but I'll leave it as a note for now but I'd say to just remove it is the better solution, even with the slider exposed by using a smaller than 1 step it's still very clumsy to edit like that, so I don't see the usefulness at al

@KoBeWi
Copy link
Member

KoBeWi commented Mar 16, 2024

exp range for particles amount was added back in dfd1331. idk if it was better back then, but I guess it doesn't make much sense anymore.

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 16, 2024

As you can see from the code the integer one didn't support exp then either so it seems to always have been dead code

It even uses the EXP_RANGE hint which didn't apply at all to the int property

* `HeightMapShape3D` had ranges configured for `float` instead of `int`
* Particles had `amount` that used `exp` which is not supported, added
  note
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I think the particles emitter's amount property makes a really strong case for actually supporting exp for integers. Particle count matters more at lower values where granular control is more desirable. That is to say, the difference between 1000 and 2000 particles is not as stark as 10 and 20.
However, that's unrelated. Beyond the comment, the PR itself fixes an obvious oversight in HeightMapShape3D.

@akien-mga akien-mga merged commit ab08876 into godotengine:master Nov 29, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the range_val_fix branch November 30, 2024 09:01
@AThousandShips
Copy link
Member Author

Thank you!

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.

4 participants