godotengine / godot

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

Rename queue_free to free in Godot 3 #12727

Closed AllanDaemon closed 6 years ago

AllanDaemon commented 7 years ago

I'm new to Godot but I'm diving very fast into it. This allowed me to see some points that could be better and the 3.0 release is intended for this.

The use of functions queue_free and free is odd to me. The default case is to use queue_free instead of free.

So I think that the functions should be renamed: queue_free -> free free -> force_free (just a suggestion).

This way, the use of this functions would be more natural, especially for newcomers and also produces a cleaner code, as queue_free usually just means free for the user.

When the original free function is really needed, you can use it with a different name, like force_free or immediate_free (the choice of the name I leave for you guys).

This way, you have the default case of free as just free and the non-ordinary one as the different named free.

IMHO this is kinda a simple change (but an extensively one). I could try to do it myself if that's the case. And it fits good in the breaking changes for GDScript 3.

Ranoller commented 7 years ago

I don't see problem in free -> force_free but queue_free -> free is not renaming, is changing the meaning to existing word, it can be problematic? Other quest: free is shorter and this is ok, but, is more descriptive?

Chaosus commented 7 years ago

queue_free means that execution of free is not happened until next frame where all queued objects frees, I dont think its a good idea to rename it. force_free instead free looks more well.

Ranoller commented 7 years ago

Can't edit on mobile (sorry for doubleposting). Other idea: renaming queue_free -> free_queue y free -> free_forced you have the two options close in autocomplete and don't lose meanings.

Chaosus commented 7 years ago

maybe rename queue_free to qfree is more ellegant solution ?

Ranoller commented 7 years ago

I think that abbreviate is not the new style of godot API... By the fact get_pos was renamed get_position... We shouldn't do random abbreviate / un-abbreviate only for the shake of do it. I like actual queue_free. But if renaming my vote goes to free_queue and free_forced for the autocomplete help

mhilbrunner commented 7 years ago

I'd vote in favor of free and free_immediate (or free_force, I don't care that much). Not q_free though, then better leave it as-is.

volzhs commented 7 years ago

I prophesy of this. there will be someone asks that...

I do free() on instance, but I can still access after free(). it's not freed after all.

Ranoller commented 7 years ago

:) :) sure...

Ranoller commented 7 years ago

Any way... renaming expressions should be completed before 3.0 launch, i'm wrong? if there are more renaming after launch... gdscript will be version 3 (cool.. Like godot :) ) and C# version 2. Faster than light...

Chaosus commented 7 years ago

should be completed before 3.0 launch

It should be completed before 20 november, when beta starts and feature freezes

Ranoller commented 7 years ago

Too much pressure to start now my "free_queue / free_force" campaign ... I'll leave it until the next elections.

NathanWarden commented 7 years ago

I agree with Ranoller in his first post. Renaming free to be more descriptive would be a good idea. I also agree queue_free should not be renamed to free as this will essentially make an API that lies to the person using it.

My personal suggestions would be: free_queued and free_immediate

This way auto-complete will offer both methods when typing "free".

edit: free_queue (without a 'd') makes it sound like you're freeing a queue of some sort instead of adding this to a queue to be freed in the future, thus free_queued seems better to me.

AllanDaemon commented 7 years ago

I think the free_queued and free_immediate logic is a good compromise. For beginners, having to figure out why 2 free methods is more logical then why there is a free method but to use it in practice you should use another to make it free.

Ranoller commented 7 years ago

I deny my own campaign but I support yours @NathanWarden and @AllanDaemon . Problem is that "the campaign" is the easy part, "pull" the campaign is more complicated (this may not be among the priorities). Some dev might be invocated to this thread to qualified opinion. @akien-mga @reduz

CowThing commented 7 years ago

Honestly I think it's good how it is. But that could just be because I'm used to it. The way I see it "free" already implies that it will be immediately gone, and queue_free means that it will be put in a list to free at a later time.

Zylann commented 7 years ago

Isn't queue_free specific to nodes though? How about objects that can only be freed? (FYI, free comes from the base class Object, while queue_free is found in Node, which inherits Object)

NathanWarden commented 7 years ago

@CowThing @Zylann I'd be fine if it stayed how it is also, but if it were to change it would be good to make it as clear as possible what they do. I can see new users being confused and always using "free" on nodes as it's obvious what it does, but queue_free isn't obvious.

However, if the example code and projects all use queue_free and if people in the forums/Q&A/Discord/etc give them correct advice they will learn to generally use queue_free instead for nodes.

So, from a self documenting API perspective changing the name is a good idea. From a practical point of view I think it ultimately doesn't matter too much. :)

djrm commented 7 years ago

new users should read what a method does before using it. also queue_free is actually quite descriptive in my opinion, of course as everything it needs you to know waht it does and when you should use it (there is no way around this).

Ranoller commented 7 years ago

The autocomplete "thing" of free_immediate /free_queue is a point to considerate too i think, it isn't?

puppetmaster- commented 7 years ago

I'm for free() and free_queue(). Not a big change and autocomplete find both.

LikeLakers2 commented 6 years ago

I don't know if free and free_queue would be good alternative names. I think they're a bit more confusing than what we currently have (free_queue as a name could imply to the user that it's freeing a queue, not queuing a free like queue_free does). Additionally, I also think it might be a good idea to keep the name the same, so that people coming from 2.1 don't have to relearn how to free a node of all things.

However, if a name change is really really needed (I don't think it is, but if it is), why not something like delete_immediately (or immediately_delete) and queue_delete? I'm suggesting this because of the name of another function we have, is_queued_for_deletion (basically, has queue_free() been called for this node during this frame?). This would not only keep the self-describing aspect of the functions and perhaps make it a bit easier for users to understand, but in turn also help users to figure out another function that they may want.

EDIT, I meant to throw this on when originally posting this: In regards to autocomplete finding both, I think that would be an even bigger mess which leaves new people wondering which does what (at least until they read the docs) and which they should use. I think it'd be better to just keep recommending queue_free (or whatever we name it), because we already describe the difference between the two in queue_free's documentation and why you'd want to queue it for deletion rather than free it right in that moment.

Oranjoose commented 6 years ago

I agree with @AllanDaemon that queue_free is confusing for beginners who neither understand the idea of a queue nor freeing memory.

However, I'd advocate to keep queue_free and free the same, because of compatibility and they are semantically correct to the operation.

That being said, I would also very much stand behind creating an alias function for queue_free, such as remove. PHP has both exit and die function to do the same thing for example. This would maintain compatibility and make the engine more intuitive to beginners.

eon-s commented 6 years ago

What is better for beginners may not be convenient, if beginners learn about how free and the deletion queue works will be better for them in the long term (and they will be beginners for a few weeks anyway, better learn the basics right).

Documentation need clear examples of free, queue_free and remove (the later is the most confusing of all IMO).

CowThing commented 6 years ago

I don't think there should be a "remove" that means the same thing as "queue_free". Since Nodes already use "remove_child" to mean removing a child node from the scene tree without freeing it.

akien-mga commented 6 years ago

I think free and queue_free are just fine. If they're confusing, it just means that more documentation should be written about them. free_immediate or free_now is redundant, as all Godot methods are run directly when they're processed, unless you call them with call_deferred (which queue_free just conveniently wraps for you). As someone mentioned above, if queue_free was renamed to free, people would start wondering why they can still access the free'd node in the same loop.

So nothing to change IMO.

Zireael07 commented 6 years ago

Documentation need clear examples of free, queue_free and remove (the later is the most confusing of all IMO).

What even is remove? First I've heard of it.

And agree fully about documentation.

eon-s commented 6 years ago

@Zireael07 I was basically referring to remove_child, but we can add remove_and_skip (which is queue_free_and_reparent_children) and all the other remove_something.

puppetmaster- commented 6 years ago

Beginner story: When I started with godot I learned to use free() to remove/delete a node, but during game jams we got crashes. Then I read about it is better to use queue_free, but also this give sometimes strange problem . Finally we implemented our own free function.

For a beginner it must be clear when to use which function and what are the known problems.

reduz commented 6 years ago

This is a common problem and I've seen it a couple of times too. I am also in favor of renaming free, but i don't think any of the naming options convince me that will avoid the problem. Maybe this is also a documentation problem..

On Fri, Nov 10, 2017 at 10:31 AM, puppetmaster- notifications@github.com wrote:

Beginner story: When I started with godot I learned to use free() to remove/delete a node, but during game jams we got crashes. Then I read about it is better to use queue_free, but also this give sometimes strange problem . Finally we implemented our own free function.

For a beginner it must be clear when to use which function and what are the known problems.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/12727#issuecomment-343473830, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z29pjy87Ngkv6f04kMma9B-sEoikKks5s1FA5gaJpZM4QVK2U .

groud commented 6 years ago

I am also in favor of renaming free, but i don't think any of the naming options convince me that will avoid the problem

Why not use destroy() ? It is short, easy to understand and it fits the purpose. It is what Unity uses.

NathanWarden commented 6 years ago

Unity actually has the same problem.

The second Debug.Log will still print the name since it has only been queued for deletion at the end of the frame.

Debug.Log(gameObject.name);
Destroy(gameObject);
Debug.Log(gameObject.name);

I think better naming is ideal, but I really don't think it's that big of a deal. Once a beginner runs into this and finds out what's really happening it will likely stick in their memory and no longer be an issue. As long as the example projects contain correct usage most beginners will likely stay with recommended coding conventions.

groud commented 6 years ago

Unity actually has the same problem.

Agree on that, still "destroy()" is more astraightforward name that "queue_free()", at least for such a common function. Also it avoids a confusion with "free()". :)

NathanWarden commented 6 years ago

I 100% agree, for beginners node.destroy() would be something I would try before trying node.free()

akien-mga commented 6 years ago

Agree on that, still "destroy()" is more astraightforward name that "queue_free()", at least for such a common function. Also it avoids a confusion with "free()". :)

No, that's super confusing IMO. destroy and free both mean "get rid of that resource and free the memory". If one is queued like in Unity and the other happens immediately, it's a mess.

If you don't like the name free we could have destroy (immediate) and queue_destroy (deferred), but I don't really see the point.

It's starting to look like bikeshedding to me. Semantically free and queue_free are correct, and the most explicit way to convey what they do (unless we nitpick on the free/destroy/annihilate naming). We just need better documentation in the classref if it isn't good enough, and maybe a tutorial in the official docs on managing nodes and resources, and explaining what are free and queue_free and when to use each. The example given above from Unity looks very puzzling, nothing in Destroy() shows that it actually happens in a deferred manner, so I wouldn't take them as reference.

Oranjoose commented 6 years ago

While it is true that once a beginner understands what queue_free means, then it is easy to associate the label with the meaning, it is only fair to also say that once a more skilled dev understands that destroy defers deletion until the end of the frame, then they too would associate the label with its meaning.

Moreover, name a popular game engine where "destroy" immediately frees the memory.

Unity's destroy defers deletion to the end of the frame (https://docs.unity3d.com/ScriptReference/Object.Destroy.html)

Unreal's destroy defers deletion to the end of the frame (https://docs.unrealengine.com/latest/INT/API/Runtime/Engine/GameFramework/AActor/Destroy/index.html)

Lumberyard's DestroyPhysicalEntity defers deletion to the end of the frame (http://docs.aws.amazon.com/lumberyard/latest/developerguide/physics-entities.html)

So in fact if destroy did immediately delete, then it would likely confuse the developer by accepted convention of the 3D game dev community. Having an alias like destroy would only serve to make the transition easier to Godot and offer a semantically more intuitive function for beginners.

NathanWarden commented 6 years ago

I agree that to those of us who are more technical that destroy and free mean the same thing, but to a less technical (more artistically minded) person destroy means "obliterate it out of existence" and free means "let it go and wander off somewhere". We really need to remember that Godot isn't just for technical people. Artists have used and will use Godot in the future.

reduz commented 6 years ago

An alternative could be to move queue_free() to Object, and rename to free_immediate() free_deferred() , so both appear to the user when trying to free stuff. This way we can better documment this behavior in both GDScript, and the free_immediate() function (where it is documented to use free_deferred() unless needed)

On Fri, Nov 10, 2017 at 2:36 PM, Nathan notifications@github.com wrote:

I agree that to those of us who are more technical that destroy and free mean the same thing, but to a less technical (more artistically minded) person destroy means "obliterate it out of existence" and free means "let it go and wander off somewhere". We really need to remember that Godot isn't just for technical people. Artists have used and will use Godot in the future.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/12727#issuecomment-343537590, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z28y7p-25NItx4yBJBDfUDDmd_aqnks5s1ImdgaJpZM4QVK2U .

LikeLakers2 commented 6 years ago

so both appear to the user when trying to free stuff

While I'd be cool with moving the function, as I pointed out in my previous comment, I feel intentionally showing both in auto-complete may be even more confusing to the user, at least to new people. Even if we note in the documentation to use free_deferred() unless you really need it, I'm not quite sure it'll be a good idea to have both show up in the auto-complete for just that reason.

Ranoller commented 6 years ago

While I'd be cool with moving the function, as I pointed out in my previous comment, I feel intentionally showing both in auto-complete may be even more confusing to the user, at least to new people. Even if we note in the documentation to use free_deferred() unless you really need it, I'm not quite sure it'll be a good idea to have both show up in the auto-complete for just that reason.

First time can be confusing, second time you know exactly what method is needed, third time (and the rest of the times) simply you got both methods near in autocomplete and the last 200 times you need to use .free_immediate/deferred are time saved. It´s usability improvement, not only new user guide.

Actually the method queue_free is a bit hidden (and in 2.1 totally undocumented), if a new user need to delete a node, probably type free, destroy ,delete ,etc... in help (results for free are now: Node.queue_free // Object.free, that´s ok) or search directly for free(), destroy(), delete(), etc... in method list... if he/she uses second method, it will not find queue_free, only free().

Last opinion about autocomplete: If the method renaming is free_deferred // free_immediate, and autocomplete follow alphabetical order, free_deferred is first choice, and that is good for new people, because this method shouldn´t crash execution throwing : "....invalid method lost_one_point_of_life() call in null instance....." :):):)

Oranjoose commented 6 years ago

@NathanWarden what I was saying in my previous post is that even "technical people" understand destroy to mean free deferred, since that's what destroy means in at least Unreal, Unity, and Lumberyard. This suggests that the convention, even among more experienced game developers is that destroy means free deferred.

And for the record, I 100% get that Godot is def not a Unity/Unreal clone and should never be. Godot is way too unique and elegant for that. At least to the extent of core features and paradigms. However, there are conventions that don't compromise Godot's specialness that should be considered. Imho destroy alias is one of them =).

NathanWarden commented 6 years ago

@Oranjoose I agree with you 100% :) and think destroy is a great option. To clarify my point, I was saying that destroy is a word that both technical and non-technical people will immediately understand (which is a very good thing), whereas free_* makes perfect sense to technical people, but may not make sense to those who are inexperienced or non-technical. I say go with the one that makes sense to as many people as possible, which would be destroy. And like you said, it's a common function/name used in almost every other game engine out there :)

But, with that said, if a person makes Godot their main engine then they will get used to whatever word is used after maybe a few weeks. Not to say that free and queue_free are bad, but almost anything mentioned above would be quite a bit better. :)

My two preferences are: 1) destroy and free 2) free_deferred and free_immediate

LikeLakers2 commented 6 years ago

First time can be confusing, second time you know exactly what method is needed, third time (and the rest of the times) simply you got both methods near in autocomplete and the last 200 times you need to use .free_immediate/deferred are time saved. It´s usability improvement, not only new user guide.

This whole paragraph seems to make the assumption that new people will always know which should be used when prompted with the choice (and in reference to your last paragraph, even if they're in alphabetical order). I'm not going to deny that with practice people will figure out which they should use, because how else do you think big name games like GTA V came out on time? What I am denying is the ideas that we can assume every new user will look at the docs (let alone even know they're there), and that even then they'll pick the "right" one (considering that technically neither of them are wrong). That feels like a pretty big assumption, one I don't think we should be making.

Of course, we could always rename them too, but then that needs to be solved somehow, something that we can't do with a simple suggestion.


Might I propose gathering suggestions for new names and throwing out a poll within as many of Godot's discussion channels (Discord, Twitter, the IRC, etc.) as possible? Making sure, of course, to include a "I don't think it should be changed" option. I'm starting to think it'd be a better idea to poll the community on this rather than let a few people choose for them, because then at least we know that the whole community knows of us considering a change of name for such an important function.

Ranoller commented 6 years ago

Might I propose gathering suggestions for new names and throwing out a poll within as many of Godot's discussion channels (Discord, Twitter, the IRC, etc.) as possible? Making sure, of course, to include a "I don't think it should be changed" option. I'm starting to think it'd be a better idea to poll the community on this rather than let a few people choose for them, because then at least we know that the whole community knows of us considering a change of name for such an important function.

"I don´t think it should be changed"option always win. I will vote for that all time, but this is not reality. I didn´t want fixed_process change, and set_pos change, set_rotd missing, OS -> engine, etc... because i´m user of godot now and this changes invalidate some of my code, but if the changes are going to happen inevitably it is better to try to give an alternative at least better than the current one, it isn´t?

akien-mga commented 6 years ago

So in fact if destroy did immediately delete, then it would likely confuse the developer by accepted convention of the 3D game dev community. Having an alias like destroy would only serve to make the transition easier to Godot and offer a semantically more intuitive function for beginners.

I don't agree. I don't have any experience with the other game engines you mentioned, and to me if I see destroy I expect it to destroy the node when the statement is run, not deferred.

Since we're all bikeshedding about what is more user-friendly for first time users (since we all agree that after using it once/reading the docs, there's no issue whatsoever with queue_free and free), I would not accept to rename queue_free to something that hides its deferred nature.

Godot's API is meant to be very explicit about what it does. If other engines are happy with less explicit names hiding the deferred status of the call, that's cool for them, but we don't have to copy them. Many first time Godot users don't have any experience with other engines, we shouldn't assume that copying Unity would make us more user friendly.

LikeLakers2 commented 6 years ago

If I may suggest a middle-ground without attempting to cause more bikeshedding, could we perhaps add destroy() as an alias for queue_free(), and label it as such in the docs? Regardless of how many users we have that don't have prior game engine experience, Godot is directly competing with other engines, and thus we can expect that a good chunk of them might be coming from those other engines - thus, I think it would be a good idea to make the transition smoother through an alias if nothing else.

akien-mga commented 6 years ago

I don't like aliases personally. Either there's an issue with the current name, and it should be changed (now that we still have the opportunity for 3.0), or it's good enough as is. If we think destroy is easier to understand than free, we could have destroy and destroy_deferred, autocompletion would make it obvious the first time you type destroy that the former is not deferred.

Ranoller commented 6 years ago

Programming in unity is hard and less user friendly than programming in godot. There are other things to copy from unity or unreal, more deeper, not the "user friendliness" that unity doesn't have. With mono we have now the unity hard way of programming, and with visual script the spagetti blueprint way of unreal. Please, don't copy from this engines more, there are better apps (spine, blender...) to inspirate in useful stuff.

LikeLakers2 commented 6 years ago

@akien-mga I'm not sure there's a better middle-ground than the alias I suggested, though

I want to avoid forcing people to look through the docs to find the new name. I'd be fine with destroy and destroy_deferred if they had the old names as aliases (or the new names were aliases to the old methods) so that people coming from 2.1 don't have to look at the docs to know how to destroy nodes... but you know how that goes.

Plus, if we're really suggesting destroy and destroy_deferred, I want to point out how likely it might be that people will bring it up as an issue in the Github and ask it to be switched around (destroy being the deferred version, and destroy_immediate being the immediate version). And I don't think it'd be right to just push away the idea because "the current names are explicit, we're not gonna change it."

I don't know what else to suggest to help this issue, because I understand both sides of it and can't justify one side more than I can justify the other. So I guess this comes back around to my suggestion to throw around a vote to our users? Maybe this time, to see if they think an alias would be useful?


@Ranoller

There are other things to copy from unity or unreal, more deeper, not the "user friendlyness" that unity doesn't have.

This user friendliness point, I'm unsure where it came from. Can someone help me with that? The only mention of Unity/Unreal/whatever was in regards to the related function being called destroy in those engines. Where does the user friendliness of other engines come into play in this conversation?

Ranoller commented 6 years ago

User friendliness related to code, we throw arguments to api changes, and i throw mine (to this case): the fact that unity have a destroy method doesn't imply that godot need that. Queue_free (or free_deferred) is more descriptive, and any person coming from unity, that can write some code, catch this in 1 second, so thinking in unity users to made api changes (if unity methods have less meaning).. Make sense?

DriNeo commented 6 years ago

IMHO the current naming make sense. free() sounds like the default destructor who simply delete the object and queue_free() is a more sophisticated function to delete objects.