godotengine / godot

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

Empty scene should still be saveable #7417

Closed ghost closed 7 years ago

ghost commented 7 years ago

Operating system or device - Godot version: Fedora 25 x64 - v2.1.1.stable.official

Issue description (what happened, and what was expected): When starting a new project and then immediately attempting to save a new fresh 3d scene, there is some cryptic error message about a missing scene root. This is super non-intuitive and something no beginner is going to care about or even understand, so the file format should be fixed in whatever way necessary to allow saving and loading even of scenes with no objects / contents to get rid of this weird corner case.

Steps to reproduce:

  1. Create new project
  2. Save new empty scene immediately (File -> Save)

Link to minimal example project (optional but very welcome):

ghost commented 7 years ago

One way to solve this could be e.g. changing the editor logic so it always has an additional, hidden root node. Or just adapt your node format to possibly contain nothing, and the loading of the scene editor to be happy with possibly loading up nothing (just for the scene editor, if the game errors out on loading up a completely empty scene I guess that is more acceptable and expected).

jacobspeicher commented 7 years ago

I think in this case it'd make more sense (if a change is even needed) to alter the alert message so that it is intuitive and understandable for a beginner. Because saving a blank scene is kind of useless, you can just make a new blank scene in the editor no problem.

ghost commented 7 years ago

There are two reasons why not allowing saving blank scenes is a bad idea:

  1. You should never in any situation bother the user with an error/warning for a special case that is just there because some oddity of the internal handling unless you really need to. However, this error message has absolutely no use except to spare the editor from handling this special case, Therefore it should go.

  2. Humans are a creature of habit. When they start a level and look at the empty scene, saving is an automatic reaction at some point. Nobody bothers at this point with pondering whether they have already added something - also, they might already have a level name in mind no matter the state of the scene which they wanted to reserve with "Save as..". An error message at this point is just jarring. (This is what happened to me, I didn't save an empty level on purpose. However, there is really no reason why I needed to be stopped, I did plan to add stuff to it later after all. Now I need to do the "Save as.." and entering the name again after adding a node, which is clearly not a good use of my time)

So I beg to differ, being able to save a blank scene with an error is absolutely not useless at all. The error throws people off, and also people might save just to reserve the name and add stuff later (or out of habit, which is a good thing - it's always better to save more often to make sure your progress is stored).

reduz commented 7 years ago

Allowing it is not possible because it does not make any sense. I suggest you read the step by step tutorials.

On Tue, Jan 3, 2017 at 12:11 AM, Jacob Speicher notifications@github.com wrote:

I think in this case it'd make more sense (if a change is even needed) to alter the alert message so that it is intuitive and understandable for a beginner. Because saving a blank scene is kind of useless, you can just make a new blank scene in the editor no problem.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/7417#issuecomment-270045943, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z2wTsfBWwkXDU9s-wBgh0bmVqaG6lks5rObx7gaJpZM4LZTI4 .

ghost commented 7 years ago

@reduz I just explained why it does make a lot of sense (in fact a lot more than not allowing it) from a usability standpoint.

I'm aware technically it might not be a useful thing to do, but the problem here is user expectations and unexpected needless error messages, not whether it makes a lot of sense to do this. Edit: might not actually have mentioned it is indeed somewhat useless technically :smile: but as I just explained here, that wasn't the point of the ticket.

reduz commented 7 years ago

Sorry, I do not agree. It will be always better from a usability standpoint that users are not allowed to do useless things. Even if he or she does not understand the error, it is good feedback to make sure the user understands that an action is not correct.

If you think it's confusing, maybe the text can be changed, but the error makes total sense.

ghost commented 7 years ago

It is not useless because it reserves a name for the level and I can just hit CTRL+S while further pondering the level and adding things ("get one thing of my list", thinking of a name). It is a useful planning tool, some people work like that (+ also the save reflex thing).

Technically an empty node might be useless, but if you really can't save an empty node and you really don't want that to be possible, why does your editor then start up with an invalid unsaveable state?

Again I'm not making this up that pretty much all editors allow you to hit instant save in whatever intiial empty state they are in, so this is a basic user expectation.

Please note I'm not saying this is a major issue or anything, and I fully understand if you're argueing this isn't worth the time to fix. However, saying this is useful or even user-friendly behavior is IMHO not very meaningful..

Edit: my brain farted some grammar a bit

Edit 2: also you are right that useless actions should be prevented - but for proper usability and UI design, the user should have clearly been prevented from entering an unsaveable state of a scene in the first place, and not be prevented from saving the scene once it happens to be in such a state (which as you say correctly should be unreachable in the first place anyway) since that sort of triggers a panic mode as a first gut reaction.

Preventing saving at any point IMHO is kind of an evil thing, and never desirable for a good user experience - even if in this specific case nothing is really lost except for the name, but it's still somewhat unexpected and does have the potential for some unnecessary initial confusion.

mswf commented 7 years ago

Can I try to clarify the workflow purpose of saving an empty scene by comparing it to programming? When I'm developing new functionality I will already have a sketch in my head of how I'll organize it in code. I then start by writing a function name, the parameters and opening and closing brackets. Then I write the code.

This is identical to how a level designer may work in production. I have a sketch of what my scene is going to be in my head. So I name my (sub)level, place it in the right folder and establish my work space by saving it. Then I start placing objects.

akien-mga commented 7 years ago

Preventing saving at any point IMHO is kind of an evil thing, and never desirable for a good user experience - even if in this specific case nothing is really lost except for the name, but it's still somewhat unexpected and does have the potential for some unnecessary initial confusion.

The point @reduz is trying to make is that a saved empty scene is actually a bogus thing to have, which will lead to a lot more frustration and bad UX than not being able to save an empty scene.

What would happen if you try to instance this saved empty scene in another scene? There is no root node to instance, so you have only two possibilities: crash, or prevent instancing the scene and show another frustrating error message. Back to square one then. Same thing if you try to load() or preload() that empty scene from a script: crash, debugger error if we're clever enough to find it out, or runtime crash of your game which raises an exception. Same thing if you try to create a new scene that inherits the empty scene. Or if you try to run the empty scene.

There are just way too many reasons not to allow saving an empty scene, as it will force us to add safeguards and error messages in a lot of places that expect that a scene is always something that is meaningful.

Can I try to clarify the workflow purpose of saving an empty scene by comparing it to programming? When I'm developing new functionality I will already have a sketch in my head of how I'll organize it in code. I then start by writing a function name, the parameters and opening and closing brackets. Then I write the code.

The same workflow in Godot would be to place a Node root node and save, those are your empty brackets. You can then change it to another type of node later on if need be, just like you would remove your empty brackets and instead make a proper class in your C++ file.

bojidar-bg commented 7 years ago

Wouldn't it be better if we show a big add node button instead of the 2D/3D viewport? Or maybe, when you click new scene, we might let you directly choose a root node, so you have nothing empty to save anyway?

kubecz3k commented 7 years ago

Or eventually inform user that's impossible to save empty scene and propose to save scene with generic Node as root.

volzhs commented 7 years ago

@bojidar-bg then... should we prevent to delete root node also?

hubbyist commented 7 years ago

bojidar-bg

click new scene, we might let you directly choose a root node.

This is a more friendly approach I think.

akien-mga commented 7 years ago

We could refactor the option to create new scenes to be a bit like the creation of new text documents in modern word processors, showing various icons + text for the available options:

Not sure how useful that would be, but if done in a way that is good-looking, it could at least feel more polished and give new users something more tangible than a blank scene.

neikeq commented 7 years ago

My opinion is the same as Akien's here. Unity scenes are not instanced inside other scenes nor inherited by them. It is way better and less error prone to just disallow it rather than adding conditions everywhere. It really makes more sense to make the error more user friendly by improving the message or even do something similar to the "no main scene defined" dialog.

ghost commented 7 years ago

Why not make a dummy node? This node would only be used specifically for saving empty scenes. Then everything would work perfectly fine, it could even be loaded and put inside another node at runtime. As soon as in the UI of the scene editor the user adds an actual proper node, that one is saved instead.

As for @neikeq I think none of what you say are good reasons for this behavior, since the main problem is that no other editor I have ever seen doesn't immediately allow saving a fresh empty project. It doesn't really matter if internally you have a good reason, it's always gonna stump and confuse newcomers. Therefore, unless it really can't be done differently without large refactoring or too much added complexity, it's simply unfavorable for usability to stick to the current behavior.

akien-mga commented 7 years ago

No, we're not going through the pain of making a dummy transitional node just to allow the convenience of saving empty scenes.

We're developing a tool for developers, they can cope with this minimal inconvenience and we don't have to implement bogus features just for a one-time quality of life improvement (any user with more than 5 minutes of Godot experience knows that they need to put a root node in a scene before saving it, just like they know they should define a main scene before running a project - and the first time, when they don't, they're told about it by a nice popup).

ghost commented 7 years ago

You can argue that way about any sort of usability issue, obviously.

akien-mga commented 7 years ago

No, there are usability issues which just imply to improve the editor, and not to implement workarounds to hide a core requirement of the game engine (i.e. that a scene is a valid collection of nodes).

Anyway, wontfix, unless someone comes up with a proposal that is not a bad workaround...

ghost commented 7 years ago

There were multiple proposals (e.g. starting up with an empty scene with any visible node that can be replaced, or having another root node in the hierarchy that is always there - which by the way is I think what blender does) that don't hide anything. Also, nothing wrong with just improving the editor, not all usability improvements need to have a good technical internal reason. (which I have tried to explain multiple times in this thread) However, I guess we're going in circles at this point.

bojidar-bg commented 7 years ago

Now you all started flaming. Calm down, the world wouldn't end to be if some idea actually turns out to be wrong or bad or ugly. Nor will it end if some idea turns out to be good, will it?

I'll reopen this issue, but, would edit it to be about hinting to the user that he must add a root node so that the scene becomes a real one, rather than just making it possible to have blank scenes that can crash everything if not handled in all parts of the code that use them. Any objections?

akien-mga commented 7 years ago

I'll reopen this issue, but, would edit it to be about hinting to the user that he must add a root node so that the scene becomes a real one, rather than just making it possible to have blank scenes that can crash everything if not handled in all parts of the code that use them. Any objections?

Sounds good, but please open a new issue for that (and link to this one for reference), it would be much clearer.

ghost commented 7 years ago

At the risk of starting a discussion again (so please feel free to ignore this entirely), what about the idea of having an always visible additional universal root node in the hierarchy of each scene? I kinda liked that proposal, and I think that is what blender does. As for the reasoning, I find it slightly favorable to @bojidar-bg 's suggestion because it doesn't require the user to do something, while idea with hinting the user about adding a node thing is again slightly more technically involved and unusual, and leaves some potentially unsaveable states - albeit more transparently of course which would be an improvement. If that is too much effort to add, then ignore this post and go ahead with the reasonable solution.

bojidar-bg commented 7 years ago

@JonasT The problem is that we can't have a nice and definite type for the "virtual root" nodes - Node2D nodes won't work inside any non-CanvasItem node. But then, Spatial nodes work properly only inside other Spatials, therefore in no CanvasItems.

Also, if you have read about scenes and nodes and instancing and whatnot, you would see why this is actually a problem -- if you instance a scene that has a virtual root, it won't inherit positions => workflow break. If you have working get_node paths, but forget to remove the virtual node => have a nice debugging time. And so on. The other problem with the change is that it breaks current workflows of existing users too much, while hardly impacting new users, until they have to learn what that "root node" actually is. Delaying learning of core concepts until things finally break is really frustrating for the user, trust me.

(bla, what am I ranting here... sorry)

bojidar-bg commented 7 years ago

Ok, move is done.

piXelicidio commented 6 years ago

I got here because I'm the real-life case of newcomer described by @JonasT
I'm new to Godot, followed the first 2 step-by-step tutorial and now was trying to start a new project from zero. First thing by instinct tried to do was to save the scene to go after to Game Settings and set it as Main Scene, then I was very confused by the "This operation can't be done without a tree root. | I see..".

My mind: What is a tree root? All this time I was reading about Nodes, then a tree root is something different? Even the message could be worded better saying "...without a root node." And the "I see.." instead of Ok was also uncommon. I go still don't understanding, becasue I thought that scene implicitly had some kind of special node with x,y,z=0,0,0 I have to google the error message, that leads me to this discussion. So all that hassle for an empty document that any other software/engine allow you to save, and we users, are used to that. This error message should exist in another place, maybe when you run the game, but nothing should stop you from saving a document on any state.

Saluds, Denys

ghost commented 6 years ago

@bojidar-bg I have another idea: what about adding a regular root node on save if there is none and give it a special flag only the editor understands, like "dummynode_removemeoneditorload", and throw it away again in the editor when the scene is reloaded? And as soon as the user adds a proper explicit root node, that one is kept. Would make it saveable, would make it work properly as subscene with a node for other scenes, and it wouldn't require special handling anywhere else than the editor itself.