Zylann / godot_heightmap_plugin

HeightMap terrain for Godot implemented in GDScript
Other
1.77k stars 160 forks source link

Godot 3.6 breaks the plugin #467

Open RockyMadio opened 1 month ago

RockyMadio commented 1 month ago

Describe the bug Several scripts will break inside Godot 3.6 stable , such as load() in hterrain_loader script , it seems like more things are affected too. But well, the entire engine feels slugish for some reasons, maybe we should not use it

Zylann commented 1 month ago

As said in several past issues at this point, it has been years that I'm no longer maintaining the Godot 3 version of that plugin. Godot 4 has been around for a long time now. If you really want to use Godot 3 with an old version of this plugin, you will have to workaround that yourself, maybe also posting an issue on Godot's repo if it broke compatibility with something.

Also, I'm lacking details from your post, but if you tried to use the version of the plugin from the main repo, it will obviously not work in Godot 3. As for the one labelled "3.3" in the asset library, this might be the last version of Godot it was maintained with. I don't recall when exactly the switch to 4 happened; it might work in 3.4 or so, but if Godot broke compatibility with anything later, it's not handled. I noticed the asset library text has become misleading so I changed it to indicate that (hasn't gone through yet)

RockyMadio commented 1 month ago

I am using 3.5.3 stable right now. I am trying to fix only this part

func load(path, original_path): var res = HTerrainData.new() res.load_data(path.get_base_dir()) return res

which is iniside hterrain_resource_loader, as Godot 3.5 complains about the function named as load(), which might conflict. Can you please let me know which other part of the plugiin uses this function, so i can find them and change accordingly

Zylann commented 1 month ago

hterrain_resource_loader.gd is actually called by Godot's resource loader, it is not called by the plugin directly. load is a virtual function from the API: https://docs.godotengine.org/en/3.5/classes/class_resourceformatloader.html, so if it were to be renamed in the script, it would not even work anymore. If Godot was changed to complain when load is used as a function name, this is a dysfunctional change (for Godot 3, at least; Godot 4 uses _ prefixes), because that would mean it conflicts with that existing API and breaks its usage.

dalexeev commented 1 month ago

The problem is not GDScript compatibility and conflict with a global function, but an incompatible change of Godot API, in 3.6 new required argument no_subresource_cache was added.

Actually, for virtual methods optional arguments do not make sense, it would require conditional passing of arguments by caller. But this really breaks compatibility and we haven't figured out how to handle it even in 4.x (a possible alternative is to use suffixes _2, _3, etc.).

The following change seems to solve the problem:

-func load(path, original_path):
+func load(path, original_path, _no_subresource_cache):