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
Export variable triggers setter #6916
Comments
There's no "inconsistency" per se, it just works the way it's intended: an external object changing the property will trigger the setter, in this case the scene being constructed sets the value. I'm don't think the export should avoid calling the setter. I expect it to call and used it like this in the past. I guess both usages are valid and there could be a way to make it optional. |
True, I get why it is this way, but I don't agree with it. What's the purpose of having the setter trigger specifically when using export(int) var a setget set_a
func _ready():
self.a = 5
func set_a():
# setter which would work the same as not using I mean let's put it this way. If I don't use |
I would have done your Globals thing differently, which is to not update it in the setter directly because most of the time you don't care. In fact, you only care when you change the scene, so it gets remembered at the moment it's switching, otherwise I would completely ignore the fact this singleton exists. That said, I'm a bit confused too by the way export works on scene loading here. I would expect that the values saved in a TSCN would be an actual dump of the state to be restored later, not something seen as "external" properties set one by one. One workaround I would use here (and I used it in my Terrain Plugin, actually), is to do this: export(int) terrain_size = 0 setget set_terrain_size
export terrain_data = [] # This is hidden through flags because it's pointless to edit in the inspector
# Simplified example, but the real-world one roughly works the same
func set_terrain_size(new_size):
if new_size != terrain_size:
# Size changed, I should add 256x256 more terrain data!
# But... not if that has been set by the scene loader. In that case,
# terrain data will be set later in the process anyways!
# But it isn't right now... we'll check in _ready(),
# so we'll not update the data twice.
if is_inside_tree():
_on_terrain_size_changed(new_size) |
@Zylann yeah, exactly, the way But what you're saying yeah... I agree, it also seems like wasted cycles if it's triggered for every property like that. |
I think part of this problem also comes from the fact
What you edit is not always exactly the same as what you save. Separating them is a way to get what you want. One meant to be edited, another meant to represent the serialized state. The editable one can be based on the other one. |
Honestly, I'd rather see the syntax made more explicit, i.e. something like:
Requiring one or the other or both to be defined. |
@Sslaxx that's not really part of the problem, it's more syntax but the behaviour is still the same. |
@Sslaxx make a separate issue for that :P, I personally prefer the current style using |
I kind of agree, but I think the issue is not that the setter is invoked when changing the value in the inspector. I think the issue is that the getter is not invoked for the default value in the inspector. I mean let's look at a typical use case for a setter / getter. Let's say you have a variable in your object that represents some size in meters. All the other values are given in meters, so it's only natural to use meters in that spot as well. But later you notice that it would make more sense to calculate in millimeters internally. So you want to change the value of your variable, but you don't want to change the values external code sees of your variable, and that includes the editor, since an inspector value of 1 should still stand for 1 meter, not for 0.001 meters.
the inspector shows a starting value of 1000, which would be the internal default value, but since that value is set at the start of the game using the setter it would in turn lead to an internal value of 1000000. |
@Warlaan, it's a valid point... So we should probably agree to either use both setters&getters in the inspector, or none at all, cause otherwise it wouldn't be consistent... |
I like the fact the inspector is able to call setters. It's incredibly handy. Why would we remove that feature? |
I agree that it's a good thing that the setter is called, but I also see the inconsistency razvanc-r complained about, since the getter is not used for the value in the inspector. For me personally it's a minor issue since the inspector value is set just once in the beginning, but I could imagine that it might be a problem in some situations. |
@razvanc-r Maybe you should change the name of the issue to "export triggers setter, doesn't trigger getter" or something to describe the issue better. |
Hmm it looks like two issues were discussed about here:
|
Does the issue persist when you just... remove the default value? |
First of all thank you for your report and sorry for the delay. We released Godot 3.0 in January 2018 after 18 months of work, fixing many old issues either directly, or by obsoleting/replacing the features they were referring to. We still have hundreds of issues whose relevance/reproducibility needs to be checked against the current stable version, and that's where you can help us. For bug reports, please also make sure that the issue contains detailed steps to reproduce the bug and, if possible, a zipped project that can be used to reproduce it right away. This greatly speeds up debugging and bugfixing tasks for our contributors. Our Bugsquad will review this issue more in-depth in 15 days, and potentially close it if its relevance could not be confirmed. Thanks in advance. Note: This message is being copy-pasted to many "stale" issues (90+ days without activity). It might happen that it is not meaningful for this specific issue or appears oblivious of the issue's context, if so please comment to notify the Bugsquad about it. |
@Noshyaar I can confirm that the issue of the setter for exported vars being called on scene load persists in 3.0. Adding to the discussion, I agree that this is less than ideal. Aside from performance issues, it can make using |
I can also confirm that setter is called on scene load in 3.0.2. |
Well, TBH there's no guarantee that the setter will be called only after everything is in the tree. Most of the time you have control over your code and can be sure it's not called before In any case, I see there's a problem but I have no idea what the solution would be. The thing I said back then still stands: an external entity setting a script variable will trigger the setter. To avoid that we would have to make special tweaks in the scene loader and script API to go around it. I still think that triggering the setter is the natural way and working around it is much simpler then the other way: if you wanted the loader to use the setter, it would be quite hard to force it to be used. Now if the Inspector could use the getter, it would solve the inconsistency. For that it would need to keep a dummy instance of the script, but would bring all the state (such as the |
As I said above, a big issue about deserialization triggering all setters one by one in an unpredictable order is that it may trigger operations quite expensively. The scene loader is not just calling setters of an entity externally, it triggers ALL of them, and for a particular reason: building the object. The whole thing is a workaround for parameterless constructors (and I'm not even talking about setters calling order!). Note: this also applies to resources, which don't have |
@vnen I did find a work-around for my particular problem. In the setter, I can check whether or not the children exist -- if they do not, then I know the setter is being called because the scene is being loaded, and they have not entered the tree yet. It's a hack, but it works. This seems to be a reliable indicator. I agree with @Zylann that it feels unnatural to consider object initialization as an 'external source" modifying the variable -- the variable doesn't even exist yet! I recognize that it's a subjective matter, but initialization doesn't feel like modification to me, so I was very surprised to see the setter being called in this case. That said, I admit to having no idea how hard it would be to change something like that in the engine. |
Well, I was mostly describing how it is, not what the ideal situation would be, just so we could think of a solution. Haven't looked at the code, but I'm pretty sure it simply calls The solution is probably to provide an alternative to |
After reading the comments here I believe that the actual problem is that the behavor (or limitations?) of the exported values are not documented. |
Note that in Godot 4.0, internal variable usage will also call the setter/getter. Feature and improvement proposals for the Godot Engine are now being discussed and reviewed in a dedicated Godot Improvement Proposals (GIP) (godotengine/godot-proposals) issue tracker. The GIP tracker has a detailed issue template designed so that proposals include all the relevant information to start a productive discussion and help the community assess the validity of the proposal for the engine. The main (godotengine/godot) tracker is now solely dedicated to bug reports and Pull Requests, enabling contributors to have a better focus on bug fixing work. Therefore, we are now closing all older feature proposals on the main issue tracker. If you are interested in this feature proposal, please open a new proposal on the GIP tracker following the given issue template (after checking that it doesn't exist already). Be sure to reference this closed issue if it includes any relevant discussion (which you are also encouraged to summarize in the new proposal). Thanks in advance! |
Operating system or device - Godot version:
Ubuntu 16.10, Godot v2.1.official.stable
export var a = 10 setget set_a
triggers setter, butvar a = 10 setget
doesn't. This might be logical from the implementation point of view but from a developer point of view it doesn't. An example why I don't like this. I'm trying to keep track of the player health in some singleton or withGlobals
when changing scenes so I do:This doesn't work when using
export
like in the above example because the setter gets triggered when the node enters the scene so theif
in_ready
will always get triggered and resets the player hp to the default value. This is unwanted and if I'm not usingexport
it works as expected.So there's an inconsistency on how the setter is triggered between using and not using
export
.The text was updated successfully, but these errors were encountered: