godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.02k stars 21.1k forks source link

Int variable without default value was incorrectly initialized with export_range annotation. #75555

Open saierXP opened 1 year ago

saierXP commented 1 year ago

Godot version

v4.0.2.rc1.official [50f26811b]

System information

Windows Vulkan API 1.3.217 - Forward Mobile -AMD Radeon(TM) Vega 8 Graphics

Issue description

extends Node2D

@export_range(1,100) var range_int:int

func _ready() -> void:
    print(range_int)

If range_int is not set to the default value, the minimum value is shown in the inspector, but range_int is initialized to 0 at runtime. If you change the value in the inspector first, and then change it back to the minimum value, range_int changes back to the minimum value at runtime instead 0.

See video:

https://user-images.githubusercontent.com/45038185/229273427-3c77bba6-d2f8-4491-a884-ffe5917f0322.mp4

Steps to reproduce

Above.

Minimal reproduction project

MPR:ExportRangeBug.zip

dalexeev commented 1 year ago

Knight-XAura commented 1 year ago

I just wanted to sneak in here and say I just run into this and my opinion really quick with a bit of reasoning...It makes sense in my eyes for the engine to use the lowest value as it's default, unless provided otherwise. however, that might be of course is up to debate.

The reason for the lowest value to show as default is that the current behavior of the engine is that if you press the reset button after you changed the value, it will still in the editor reset back to in the provided example: 1, even though it'll now be 0 instead when accessing the variable. I think it'll be massively confusing if we just try to alert the user in some way because if you have a lot of these it could get hard to manage if you don't just have it set within the needed range.

You could argue that setting by default in the asked for range to its lowest value could cause code to work, but wasn't intended as maybe they forgot to then set the number to something else altogether, but there is another side to that argument where the code still works anyway, and they still don't know. Although then you could argue that their code should better handle incorrect/unexpected values, but I think you get my point.

Either way I think the proposed solution so far is still a good one, just not a matter of my personal preference and so I figured I'd throw my opinion into the ring as I'm sure it's not an uncommon opinion. I can't wait for whatever solutions comes of this! :)

Sch1nken commented 2 months ago

To add to this (since this just came up on the german godot discord): This also affects Custom Resources with @export_range. Creating a new Resource and not changing it (the exported value) in the inspector results in "0" being written to the .tres

CaptainFacepalm commented 2 months ago

Just experienced this issue for the first time in a resource where I had only set the range on an int and not a default starting value with "=". Reading the initial replies to this I honestly can't say I agree with any proposal that suggests the default value should be anything other than the value the inspector shows a user. If a user can't trust what the inspector is showing them, the inspector isn't doing its job correctly.

Adding to the confusion, hitting the 'reset to defaults' arrow next to a value resets it to the range minimum too, so to be given a value of 0 even after that is extremely confusing.

I think the best solution is just to set the default value to the minimum range value because when someone makes a range, their usual intent 99.9% of the time is to restrict a value to only fall between those min and max values. Making the variable = anything that isn't within the range seems to be against intended UX functionality.

whiteshampoo commented 2 months ago

Personally, I would not see it as an error if it defaults to 0, even if the range has a higher minimum. However, the inspector MUST show the correct value. I would also print a warning if the default value is outside of the export range. (Maybe even force the programmer to define a default value?) I'm not sure if it is correct to default to the minimum. Why not the maximum or the middle? (IMHO)

If you really want to make sure teh value is always in the specified range, you can use setter/getter:

const foobar_min: int = 13
const foobar_max: int = 37

@export_range(foobar_min, foobar_max) var foobar: int:
    set(fb): # optional
        foobar = clampi(fb, foobar_min, foobar_max)

    get():
        return clampi(foobar, foobar_min, foobar_max)
CaptainFacepalm commented 2 months ago

I think if you're setting a range, you just expect that range to be enforced. As most variable types will default to 0 (or thereabout), defaulting to the lowest possible value within the range seems fitting. Whereas I think most users would expect to have to set a default value if they wanted it to start midway through the range or maxxed out.

As for making a getter/setter for the value, while it would work, it feels like cumbersome extra steps that don't fix (what I would argue) is an underlying UX issue.

One last note on the alternative where the inspector shows the 'correct' value and displays a 0 with a warning message. I wonder what is being achieved there that doesn't boil down to a user maybe being confused by the message, and then just setting the default value? It doesn't seem advantageous to the UX of the engine to give users extra steps in this regard.

BlueLumin commented 1 month ago

Just wanted to add that I also experienced this issue and it took me a solid minute to work out what was wrong. I feel that because in the editor it's showing the minimum of the range as the default, you expect that to be the value set for that variable.

I recreated the issue in a MRP which I've attached below: export_range-bug-report