Fix Curve min/max limits being bypassed on first assignment.

[anvilfolk]

This is an alternative PR to #78931 - see that PR for problem description. It does add a step to resource loading in general, but in doing so provides a simple hook for post-loading logic to be executed in any resource.

This enables Curve - or any other resource with mutually dependent variables - to have a simple _is_loading variable, which toggles whether setters enforce dependencies/restrictions (no during loading, yes otherwise). This means we don't need workaround "fake" properties like in #78931, which increase code complexity & number of properties.

[anvilfolk]

@ttencate I believe this is similar to what you suggested here and in RocketChat. Pulling the post-load hook into ResourceLoader allows us not to have to call the hook in each subclass of ResourceFormatLoader, which someone would surely forget at some point, but does mean that we do not have a place to call a pre-load hook.

I do think maintainers might not be excited at how not-local this solution is, but we'll see! Perhaps other folks that have run into this problem will come out of the woodwork and say they wish there was something like this for more resources than just Curve :)

[ttencate]

Thanks for taking the time you said you didn't have! Needless to say, I like this better 🙂

Could it be a notification instead of a virtual function? That's slightly less intrusive perhaps. Consider exposing the notification constant to GDScript as well.

But... how is _is_loading set to false if the Curve is newly created? Don't we need a pre-load function/notification for that? [Edit: i.e. for setting it to true in the first place.]

[anvilfolk]

Thanks for taking the time you said you didn't have! Needless to say, I like this better 🙂

Managed to push through the depression for a little bit, it felt nice to work on code again :)

Could it be a notification instead of a virtual function? That's slightly less intrusive perhaps. Consider exposing the notification constant to GDScript as well.

During the discussion in RocketChat, some maintainers said that notifications/signals are much heavier weight, and should be avoided unless there are very strong arguments for having them. I'm not sure we have those strong arguments. The issue is present in GDScript Nodes, but this approach can be used there as well if/when necessary as follows:

var loading = true
@export var max_var : int:
	set(value):
		if value > min_var or loading:
			max_var = value

@export var min_var : int:
	set(value):
		if value < max_var or loading:
			min_var = value

func _ready():
	loading = false

Not entirely sure we can fix it with Resources since they don't have a _ready() functions 🤔

But... how is _is_loading set to false if the Curve is newly created? Don't we need a pre-load function/notification for that? [Edit: i.e. for setting it to true in the first place.]

This is a good point. Right now, the variable is initialized to true by default, which means any object, including those that aren't loaded but are manually created with e.g. var my_curve = Curve.new() in GDScript will be tagged as "loading" and the restrictions won't apply.

So we need to find a way to have that pre-load hook after all!

[anvilfolk]

Set this as draft, as it's currently in proof-of-concept stage, but does seem to work.

Would love some input from the team on whether this feels like a good direction to go in.

Here's the updates:

• Added LoadHookResource for resources which have preload/postload functions, and changed Curve to this type. Loading hook functions are called at the appropriate places in ResourceLoaderText::load() for the main resource & subresources.
• Refactor out the code for parsing resource properties, which was basically duplicated for subresources & main resource. It is now in ResourceLoaderText::_parse_resource_properties() instead of inline in ResourceLoaderText::load().
• This refactor also gives us a single point (after _parse_resource_properties() was called) to check whether the properties were loaded successfully, after which we can call _finish_loading().

Would still need call the hooks in ResourceLoaderBinary, but I feel like this approach at least won't cause Resources in general to behave inconsistently. If other resources want to have hooks, they would subclass LoadHookResource and then make sure the appropriate loaders supported that.

Having GDScript support for this would also make sense, but we can dig into that if we feel this is the way to go.

[ttencate]

What's the reason for having this intermediate LoadHookResource subclass – i.e. why not put _start_load and _finish_load directly on Resource instead?

The drawback of an intermediate subclass is that classes further down the inheritance chain can't opt in to receiving load hooks if their parents don't. (Unless we use multiple inheritance, but I'm not sure the engine supports it, and it's a can of worms we'd rather not open.) For example, if RectangleShape2D needs it, its parent class Shape2D would have to be changed to inherit from LoadHookResource.

[anvilfolk]

Honestly, I'm pretty unhappy with every solution we've come up with. None of them feel good. I'm almost back to just thinking having dummy properties is the way to go, despite it being a hacky work-around😭

I'd really like to avoid Resource having those methods, since you'd expect them to be called for all resources. But for that to work, we'd need to implement it for 18 different loaders just because Curve needs it. This contradicts Godot's design philosophy of local solutions for local problems. With this subclass we can at least isolate the solution more specifically to the resources that actually need it.

@akien-mga mentioned that Range had a similar problem, but that's a Control, not a Resource, so now we need to add this logic to maybe scene deserialization and find a place for those hooks somewhere in that hierarchy? It's just a mess 😭

[anvilfolk]

Or maybe we go back to thinking about property hints or something and find a way to bypass regular setters there...

[anvilfolk]

Closed as it is an unsatisfactory solution.