Disable `display_folded` and `editable_instance` in release builds

[Mickeon]

See also https://github.com/godotengine/godot/pull/68560.

This PR dummies out is_displayed_folded(), set_display_folded(), is_editable_instance(), set_editable_instance() of Node to do nothing on release builds. In the editor, they work completely fine still, as they're intended to.

I do not expect anyone to have ever used these in release builds, and if they do... I'm not sure why? This is technically compatibility breaking, but in practice you shouldn't use these methods outside of an editor context, nor you probably have ever tried to, because the documentation heavily implies they're not intended to be.

[fire]

Isn't the set_editable_instance the way Godot uses to tell what an instanced scene is?

What is the usecase of this? If it is a performance optimization what does it make faster to trade complexity in debugging?

[Mickeon]

Isn't the set_editable_instance the way Godot uses to tell what an instanced scene is?

Not quite. It corresponds to this checkmark:

What is the usecase of this?

In release builds? Practically none. Theoretically, users that may need this would be making their own implementation (perhaps with metadata). Probably, they do not even know these bits are still available on release builds, for reasons stated above.

If it is a performance optimization what does it make faster to trade complexity in debugging?

Saves two bits, literally. But, maybe keeping it in debug builds? Maybe someone chucks in their own, external debug viewer, somehow, and may need these two bits...?

[Mickeon]

Refactored this PR to make it resemble what the SceneTree does for a very similar purpose. It's very nice, clean, I love it. For reference: https://github.com/godotengine/godot/blob/91fcc3986eca5331b4cc44a67adcc2f29a49eb66/scene/main/scene_tree.cpp#L664-L669 https://github.com/godotengine/godot/blob/91fcc3986eca5331b4cc44a67adcc2f29a49eb66/scene/main/scene_tree.h#L296-L300

[akien-mga]

There have been at least 4 PRs attempting to expose editable_instance before one was eventually merged, so I think users would disagree with you that this is not useful at runtime: #52000. CC @lyuma

[Mickeon]

It's one thing exposing it and the other is keeping it for Release builds. Perhaps only debug builds?

[dustdfg]

I really don't see conflicts. I am not very knowledgeable but I didn't find any places in all the interlinked PR's/Issues where these methods are used outside editor... I mean I saw many arguments that it would be useful for tool scripts. Yeah it is great but tool scripts are run inside editor. Aren't they? This PR disables these methods for non editors.

Hello @lyuma. I saw you pushed exposing these methods so you should know better than I... Please, say where these methods are used in the exported game? Outside editor?

[Mickeon]

Rebased. Both flags now require DEBUG_ENABLED instead.

[Ivorforce]

This makes sense to me. I'm guessing the wish to expose this property pertains only to editor tooling (#52000 does at least). I'd probably go one step further and fully remove the functions for release builds, to fail statically if non-editor code tries to call it. But please take my opinion with a grain of salt; I hardly know the code or anything surrounding it. Perhaps it would be better reviewed by editor folks?

[Mickeon]

I'd probably go one step further and fully remove the functions for release builds

Can't do, unfortunately. These methods still need to be bound to ensure user code cannot be unexpectedly screwed up in released builds, so they're basically assigned to dummies. There's other methods like these that are handled similarly.

I could implement an always-present error when they're called in release builds, but I'm not sure there's any precedents like it.

[allenwp]

Is there any way to have non-editor builds always behave the same? Like putting an editor hint check in the function body? I'm not understating why debug non-editor needs to behave like the editor...

(Importantly, keeping code paths as similar as possible between release and debug non-editor is helpful to users...)

[Mickeon]

I'm not sure what you asked. Could you elaborate? It seems like, as by the above sentiment, these methods may actually be used even in debug builds (for some reason).

[allenwp]

Hmm. I don't know much about the subject matter here, so maybe there is something I am misunderstanding.

My comment is a general development best practice to minimize bugs/behaviours that can only reproduce in final release exports of a game project:

If there is any difference in behaviour or code paths between non-editor debug exports and non-editor release exports, it introduces a risk for a bug/different behaviour that only reproduces in the release export of a game project. For example, maybe a debug export errors out in some cases, causing user scripts to fail and the game is tested by the game developer to work well in this case (usually the developer just doesn't notice that an error is thrown). But then in release export, the script doesn't fail and execution follows a different code path that results in different release export behaviour in user scripts, which ultimately leads to a bug because the game developer never discovered this behaviour during development with the debug export.

So for this reason, it's best that non-editor debug and release behave as closely as possible... That said, I've noticed this is not the paradigm that Godot uses throughout its codebase, which contributes to some release-only bugs. My belief is that any difference between release and debug code in non-editor exports is generally undesirable unless there is a performance reason for this or because it will prevent a crash in the release export. Said differently, if the code can't crash in release, then it should behave the same as debug unless there is a real performance reason for the difference.

In the case of this PR, maybe none of this applies at all, so my apologies if that's the case.

[Mickeon]

Your sentiment actually does apply to this PR as well. However, for reasons argued above, using these methods outside of a debugging context is practically useless. In my opinion, relying on these for important behavior in a release application can be seen as bad practice, too.

So for this reason, it's best that non-editor debug and release behave as closely as possible... That said, I've noticed this is not the paradigm that Godot uses throughout its codebase, which contributes to some release-only bugs.

I recall this was discussed recently. Ideally, Godot should actually follow this paradigm. There's many such cases that need to be addressed.

[allenwp]

OK, I agree this is bad practice for a game developer to use these functions outside of the editor conext simply because it's pointless to do so.

But I don't understand what issue this PR is aiming to resolve?

If the goal is to prevent code from executing that makes no sense to execute, then I believe the correct fix would be to add an "is editor hint" check at the beginning of the functions to return early rather than creating a discrepancy between release and non-editor debug exports.