godotengine / godot

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

Instanced inherited scene not updated after collision layer change in base scene #43032

Open timothyqiu opened 3 years ago

timothyqiu commented 3 years ago

Godot version: 3.2.3 & current master (d5073c6)

OS/device including version: not graphic related

Issue description: After changing the collision layer or collision mask of base scene, inherited scene's default collision layer / mask updates correctly. But instanced derived scene node's default collision layer / mask is not updated until I reopen the scene.

Steps to reproduce:

  1. Setup the project (or use the MRP)
    1. Create a new scene with KinematicBody2D as root named Base (collision layer 1 is on by default)
    2. Create a new inherited scene from Base, name it Derived (collision layer 1 is on by default because of inheritance)
    3. Create a new 2D scene named World
    4. Instance Base and Derived in World (collision layer 1 is on by default)
  2. Turn off collision layer 1 in Base
  3. Inspect the collision layer and collision mask in other scenes
    • In Derived scene, collision layer 1 is off and not resetable
    • In World scene, the Derived node still has collision layer 1 on, and the collision layer is shown resetable.
      • After close & reopen the World scene, collision layer of Derived node is shown correctly (layer 1 off, not resetable)

Minimal reproduction project:UpdateCollision.zip

hilfazer commented 3 years ago

I had a look at the issue. The problem is not specific to collision_layer, it happens with other properties as well.

I've tried a couple of scenarioes: a scene inherits base scene - no problem a scene instances a scene that inherits base - problem occurs a scene instances a scene that instances base - no problem a scene inherits a scene that inherits base - problem occurs

I concluded the problem has something to do with scene inheritance. And that's indeed the case.

There are 2 bugs here. One minor and one major. The function EditorData::_find_updated_instances() doesn't return true in some cases it should. Those cases are switching the scene to one of the following: a scene instances a scene that inherits base a scene inherits a scene that inherits base

which are the cases when OP's problem occurs. So if that function is fixed the problem will go away, right? Let's make a temporary workaround - ignore the function's result and enforce scene's reload on every switch. Aaaand...

... the problem is still there ):

The second problem is the major one. It's not easy to explain but i'll do my best. I'm going to assume _find_updated_instances() works correctly.

Let's say we have following scenes: Base.tscn Derived.tscn (inherits Base.tscn) WithDerived.tscn (instances Derived.tscn)

All three are opened in the editor. User changes something in Base.tscn and saves it. They switch to one of the other scenes. That calls EditorData::check_and_update_scene(). The function determines the scene the user switches to needs a reload. That means calling those 2 functions:

PackedScene::pack(Node *p_scene)
PackedScene::instance(GenEditState p_edit_state)

PackedScene::pack() calls SceneState::_parse_node(). This function tries to determine which properties need to be saved - it compares values from Node to those in PackedScene. It also checks properties from base scenes (if there are any).

Let's start with collision_layer with value of 3. User changes it to 7 and saves Base.tscn. Then they switch to Derived. Value of collision_layer tht sits in Node (3) gets compared to previous value from Base.tscn (3). Since they are same Godot determines the property is inherited from Base and doesn't save it. Good. Now the user switches to WithDerived. Node's value of 3 gets compared with... new value of collision_layer from Base.tscn! That value is 7 so Godot concludes it's been overridden and saves it. Not good.

But why is there a difference? I'll explain. Node has fields named instance_state and inherited_state. For Derived.tscn inherited_state corresponds to Base.tscn. This data seems to be exclusive to Node that holds it, it doesn't change when Base.tscn gets saved. It allows SceneState::_parse_node() to access previous property values from Base and save properties correctly.

For WithDerived.tscn instance_state corresponds to Derived.tscn. But how does this node know about data from Base.tscn? One of values in variants vector, usually (always?) with index of 0, is a Ref<PackedScene> that corresponds to Base.tscn but...

this reference is not exclusive to a given Node - it's shared. It points to updated Base.tscn. Which is the reason why WithDerived has access to updated data from Base too early (during PackedScene::pack() call).

Let's have a look at the function that saves a scene: EditorNode::_save_scene(String p_file, int idx). This line: sdata = Ref<PackedScene>(Object::cast_to<PackedScene>(ResourceCache::get(p_file))); fetches a Ref that points to the same object as Ref in variants[0] of inherited_state/instance_state fields of Node.

It doesn't seems to be done by accident but rather it's intended, based on this comment:

        // something may be referencing this resource and we are good with that.
        // we must update it, but also let the previous scene state go, as
        // old version still work for referencing changes in instanced or inherited scenes

Alas the old scene state gets erradicated instead.

KoBeWi commented 2 years ago

Related to #28090

SamDevelopsCode commented 3 months ago

Related to: https://github.com/godotengine/godot/issues/93365

messhkod commented 2 months ago

Can confirm problem still occurs in v4.2.2.stable

Temporary workarounds:

  1. Set desired values in Parent Node _init() or _ready()

OR

  1. Reset values for each instance (not realistic) :

Go to scene with problematic instances. Select one of them. Reset value to default in Inspector Now repeat that for each instance :)

messhkod commented 2 months ago

Can anyone confirm this problem is worked on?

I'm not that technical and don' know how underlying code works. But shouldn't default values be pointers to Base parameters, and store explicit value only when overwritten?