Skip to content
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

Mono: Base class members reset to their default values if derived class has [Tool] #37812

Closed
Phischermen opened this issue Apr 11, 2020 · 22 comments · Fixed by #56299
Closed

Comments

@Phischermen
Copy link
Contributor

Godot version:
3.2.1.stable.mono

OS/device including version:
Windows 10

Issue description:
C# base class members reset themselves to their default values every time the project is built if the derived class has the [Tool] attribute. I have not been able to reproduce the issue in gdscript.

Steps to reproduce:

  1. Download the sample project.
  2. Set exported variables to something other than their defaults. "B" is from Base.cs and "D" is from Derived.cs.
  3. Make some changes to either Base.cs or Derived.cs (other than changing the [Tool] attribute) so that the project has to be rebuilt.
  4. Press build.
  5. Check "B." It should have reset its value.

Minimal reproduction project:
DerivedC#BugTest.zip

@Paha-Zul
Copy link

I'm having the same problem but I didn't make an issue because I couldn't reproduce it reliably. It's super annoying now because I've converted a lot of scripts to C# and it's resetting (seemingly) random sets of nodes in my scenes.

For instance, if I have a wall, a couch, a vending machine, and a sink, it will sometimes decide to reset just the walls and the couch but not the others. They all derive from the same classes and all use the [Tool] functionality. Hope this gets looked at!

@Phischermen
Copy link
Contributor Author

@Pahasz Do you think you are now able to reproduce the issue by following my instructions?

@Paha-Zul
Copy link

@Phischermen Yes I was able to reproduce it in the reproduction project using those exact steps.

@neikeq neikeq added this to the 4.0 milestone Apr 28, 2020
@neikeq neikeq added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 28, 2020
@neikeq neikeq self-assigned this Apr 28, 2020
@reimgrab
Copy link

reimgrab commented May 28, 2020

Same here and it is super annoying. Cannot really use the Godot editor to build test levels anymore as I could when my game was still written in GDScript.

Btw. the bug is cross plattform as I am running Linux.

@Paha-Zul
Copy link

Any update on this?

@ksvslk
Copy link

ksvslk commented Jul 31, 2020

Any update on this?

Also interested in the solution of this issue.

@Phischermen
Copy link
Contributor Author

There is this post in the Godot Devblog. It seems that after Ignacio organizes the Godot API into namespaces and provides a .NET friendly IO API for the Godot virtual file system, he will dedicate a month to bug fixing

@rexfleischer
Copy link

this is also happening on 3.2.2.stable.mono

@Paha-Zul
Copy link

Paha-Zul commented Aug 8, 2020

It's been happening from 3.2.1 through 3.2.3 beta-1 for me.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 10, 2020
@Scripptor
Copy link

Happens on 3.2.3.stable.mono
It only seems to affect scenes that are currently opened, active or in the background!
So if you want to save your work and you have a level with inherited tool scenes with script variables, please close the level scene before building! This might just save you some time!

@Paha-Zul
Copy link

Still happens on 3.3 RC 6. Tested with the sample project to confirm.

@Drkie
Copy link

Drkie commented Dec 7, 2021

Still happening on v3.4.stable.mono. This heavily discourages the use of [Tool] because of how easy it is to lose data in all your open scenes without any warning. Even innocuous changes to the class such as adding a comment will trigger this bug.

@Krumelur
Copy link

Krumelur commented Dec 14, 2021

Also seeing this here with 3.4 stable and 3.4.1 RC2. My observations, maybe they help:

  • It seemingly never affects boolean values. But I see it with string and Vector2.
  • It only affects exported properties, not member variables.
  • It does not matter if a backing field is used or not.
  • The set isn't called when the resetting happens. Suddenly, the default value of the backing field is returned from get.
  • The entries for the property values in the TSCN file disappear (are removed), this can be seen when using source control.
  • It happens only sporadically. I've been coding for 2h without a problem and now most of my property values are gone.
  • It seems to affect [Tool] decorated classes that derive from custom base classes.

When can we expect #53975 to land on 3.4, as this - as I interpret it - addresses the problem?

@raulsntos
Copy link
Member

I don't think #53975 fixes this issue, that PR was made to fix an issue that only affected master, it was caused by #44106 which was only merged in master.
Later, when #44106 was being backported to 3.x, the issue was found and #53975 was made to fix it. Both PRs were included in the backport to 3.x: #53942.
The backport PR has already been merged and will be included in the future 3.5 release.

@Krumelur
Copy link

Thank you @raulsntos! What’s the timeline for 3.5 and do you have a workaround for the meantime, even if it means changing some code?

@Calinou
Copy link
Member

Calinou commented Dec 15, 2021

What’s the timeline for 3.5

There is no ETA for Godot releases, but since 3.4 was recently released, I expect 3.5 to be released in about 5 months from now.

@raulsntos
Copy link
Member

Do you have a workaround for the meantime, even if it means changing some code?

You can workaround it by closing the scene before building, as described by Scripptor: #37812 (comment)

@DwarfNinja
Copy link

DwarfNinja commented Dec 15, 2021

Do you have a workaround for the meantime, even if it means changing some code?

The current workaround I've been using is to convert the C# Resources to GDScript and create a C# wrapper for them.
Here's an example:

Example of a Resource wrapper class
//The wrapper class of a resource
public class MyResourceWrapper {

    private Resource resource;

    public Resource Resource {
        get => resource;
        set => resource = value;
    }

    public MyResourceWrapper (Resource resource) {
        Resource = resource;
    }

    public static MyResourceWrapper LoadResource(string filePath) {
        return new MyResourceWrapper (ResourceLoader.Load<Resource>(filePath));
    }

    public string MyProperty1 {
        get => (string) Resource.Get("MyProperty1");
        set => Resource.Set("MyProperty1", value);
    }
    
        public int MyProperty2 {
        get => (int) Resource.Get("MyProperty2");
        set => Resource.Set("MyProperty2", value);
    }
}

PS. In practice I have the Resource field/property and constructor delegated to an abstract class.

When needing to use a Resource in a class I export a Resource property and use the get/set to handle the correct conversion/wrapping. (Perhaps not the the cleanest, the set/get functionality could also be delegated to the constructor but you get the idea).

Example of a class that uses a Resource
 //Class that uses a Resource
    [Export] 
    public Resource MyResource {
        get => myResource?.Resource;
        set {
            myResource = value switch {
                null => null,
                MyResourceWrapper resource => resource,
                _ => new MyResourceWrapper(value)
            };
        } 
    }
    
    private MyResourceWrapper myResource;

This main advantage from this solution is that it allows me to use the ResourceWrappers like they are real C# Resources and keeps refactoring of my code minimal. This also futureproofs for when the fix releases, as all I need to do is switch the GDScript resources back to C#, and use those instead of the wrapper.

@Krumelur
Copy link

Krumelur commented Dec 18, 2021

What replacing all properties with class member variables also work? Or would they be reset randomly, just as well?

Also, can we vote to get a 3.4.2 and include the fix there? The workarounds are not really options IMHO and waiting months for 3.5 isn't a pretty alternative :-(

@Calinou
Copy link
Member

Calinou commented Dec 18, 2021

Also, can we vote to get a 3.4.2 and include the fix there? The workarounds are not really options IMHO and waiting months for 3.5 isn't a pretty alternative :-(

Since #53942 doesn't break compatibility with existing projects, it should be safe to cherry-pick to 3.4 to be included in 3.4.2 RC1.

In the meantime, you could build a Mono-enabled editor and export templates from source with #53942 included.

@erayzesen
Copy link

erayzesen commented Dec 26, 2021

I have struggled a lot in the past with this problem, I'm glad that this error will be fixed in the next version.

I suggest you a safe solution, it was always worked for me. Define your main variable as a virtual variable in base class and re-define your variable as override .

Example;

Base class;

public virtual float myVariable{get;set;}
[Export]
public float myVariableConfig{
  get{return myVariable;}
  set{myVariable=value; }
}

Re-define your variable in the derrived class;
public override float myVariable{get;set;}

When we define the variable in this way with "override", it treats its as a special variable that is not specific to the base class. So we're fooling Godot.

@raulsntos
Copy link
Member

raulsntos commented Dec 28, 2021

I'm sorry, I don't think I made myself clear. This issue is not fixed in master or 3.x, I meant to say that the mentioned PR is unrelated and does not solve this issue but a different one.

This issue should be fixed by #56299 in master and by #56300 in 3.x if they get merged.

@akien-mga akien-mga modified the milestones: 4.0, 3.5 Dec 29, 2021
DwarfNinja added a commit to DwarfNinja/DwarfGame-Project that referenced this issue Jan 6, 2022
Built custom Mono Godot Editor which fixes Issue godotengine/godot#37812 that previously required use of the ResourceWrapper class to fix.

Version: 3.4.2-stable

Added Commit: godotengine/godot@907e709

Related Pull Request: godotengine/godot#56300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.