godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix some invalid `int` property ranges

Open AThousandShips opened this issue 1 year ago • 7 comments

  • 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 avatar Mar 16 '24 11:03 AThousandShips

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).

Mickeon avatar Mar 16 '24 12:03 Mickeon

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

AThousandShips avatar Mar 16 '24 12:03 AThousandShips

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

KoBeWi avatar Mar 16 '24 12:03 KoBeWi

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

AThousandShips avatar Mar 16 '24 12:03 AThousandShips

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

AThousandShips avatar Mar 16 '24 12:03 AThousandShips

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

KoBeWi avatar Mar 16 '24 21:03 KoBeWi

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

AThousandShips avatar Mar 16 '24 21:03 AThousandShips