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

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

Open
saierXP opened this issue Apr 1, 2023 · 8 comments

Comments

@saierXP
Copy link

saierXP commented Apr 1, 2023

Godot version

v4.0.2.rc1.official [50f2681]

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:

export-range-bug.mp4

Steps to reproduce

Above.

Minimal reproduction project

MPR:ExportRangeBug.zip

@dalexeev
Copy link
Member

dalexeev commented Apr 1, 2023


@Knight-XAura
Copy link

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
Copy link
Contributor

Sch1nken commented Aug 5, 2024

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
Copy link

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
Copy link

whiteshampoo commented Aug 6, 2024

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
Copy link

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
Copy link

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

@nanodeath
Copy link
Contributor

I opened #98092 which ended up being a dupe of this, but wanted to add that this also affects C# classes in the same way.

Custom resources act almost identically, except that the default value (e.g. 0) gets serialized into the scene files as well, which doesn't happen for normal custom classes extending Node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests