Redot-Engine / redot-engine

Redot Engine – Multi-platform 2D and 3D game engine
https://redotengine.org/
MIT License
4.8k stars 218 forks source link

[Godot] Unable to assign Skeleton with Import Script #158

Open scotmcp opened 1 month ago

scotmcp commented 1 month ago

Tested versions

This is taken directly from: https://github.com/godotengine/godot/issues/95461 which included some discussion and steps to resolve, but it was not actioned and REALLY should be.

Please go to the link to read the discussion.

Reproducible in all 4.x versions.

System information

Godot v4.3.rc3.mono - Linux Mint 22 (Wilma) - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 4060 Ti (nvidia; 535.183.01) - AMD Ryzen 7 5700X 8-Core Processor (16 Threads)

Issue description

The ConfigFile class set_value method erases key/value pair when value is set to null (this is per class docs for ConfigFile.set_value). However, some files such as .glb.import files have required keys stored with explicit values for some options.

There is already the method erase_section() and erase_section_key(), and we need the ability to write null values to key/value pairs.

Example for when we need to be able to write an explicit null:

[params]

{_subresources={
"nodes": {
"PATH:Armature/Skeleton3D": {
"rest_pose/external_animation_library": null,
"retarget/bone_map": Resource("res://explosive_bone_map.tres")
}
}
}

If this value is not set, then the entire PATH:Armature/Skeleton3D node is over written by the engine on the next import. Thus an entry to set skeleton ex. {"nodes" : {"PATH:Armature/Skeleton3D": {"retarget/bone_map": Resource("res://bone_map.tres")}}} will also be erased and unset.

To justification that a change is needed:

If one copies and pastes a correct but partial entry, the editor will erase the incomplete entry upon next import...example:

_subresources={
"nodes": {
"PATH:Armature/Skeleton3D": {
"retarget/bone_map": Resource("res://explosive_bone_map.tres")
}
}
}

becomes: _subresources={}

If one copies and pastes the complete correct node entry with the required null value, the editor is fine. example:

_subresources={
"nodes": {
"PATH:Armature/Skeleton3D": {
"rest_pose/external_animation_library": null,
"retarget/bone_map": Resource("res://explosive_bone_map.tres")
}
}
}

Therefore one must conclude that the Class used to read and write configuration files such as .import files, should be able to write key/value pairs that include explicit null values.

There is already the method erase_section() and erase_section_key(), and we need the ability to write null values to key/value pairs.

Minimal reproduction project (MRP)

Animation Test.zip

EDIT: I said FBX by mistake up above, I corrected that to GLB....I am not sure if that's important or not...I would expect the import file behavior to be the same for either.

Steps to reproduce

Steps to reproduce

@tool # Needed so it runs in editor.
extends EditorScenePostImport

var config = ConfigFile.new()

func _post_import(scene):
    set_skeleton()

func set_skeleton() -> void:
    var bone_map : BoneMap = load("res://explosive_bone_map.tres")
    config.load("res://relax.glb.import")
    var rest_pose_node : Dictionary
    rest_pose_node = {"nodes" : {"PATH:Armature/Skeleton3D": {"rest_post/external_animation_library": null}}}
    config.set_value("params", "_subresources", rest_pose_node)
    config.save("res://relax.glb.import")

Minimal reproduction project (MRP)

N/A

scotmcp commented 1 month ago

I don't understand why this issue got merged with one that seems completely unrelated.

jawbroken commented 1 month ago

why are you reporting real bugs here? the guy who made this fork can't code or fix them. report them to the real project

scotmcp commented 1 month ago

why are you reporting real bugs here? the guy who made this fork can't code or fix them. report them to the real project

thanks for the troll

jawbroken commented 1 month ago

it's not a troll, just a statement of fact. hence why all the fork has done so far is change Godot to Redot in a few places, fail to have a working website, and fall behind the real project

scotmcp commented 1 month ago

And your comment is the same as someone yelling insults from the bleachers...it's just a lot of noise

Find something more useful to do with your life than harrass and troll.

This project will succeed or fail on it's own merits, it really doesn't need your help to do either.

jawbroken commented 1 month ago

i'm just trying to help these poor guys out who think reporting bugs on here is anything but an insane waste of time. that's not trolling or harrassing ✌️

scotmcp commented 1 month ago

@scotmcp Please be respectful of jawbroken. You may be in violation of the CoC and be removed from the opening issues with this project.

Am I misunderstanding something? You do not want me to report bugs? And it's ok for someone else to say, "don't bother reporting bugs, they can't fix them?"

Something seems off here...

jawbroken commented 1 month ago

it's okay, you didn't know i was just trying to help. a simple apology from you should smooth things over and we can both move on

scotmcp commented 1 month ago

I have to be honest. I don't know how this is helping anyone.

jawbroken commented 1 month ago

actually, now i can see you've called your friends in here to brigade all the posts with votes, which is a bannable offence. enjoy your timeout

dandykong commented 1 month ago

Says the guy upvoting himself.

jawbroken commented 1 month ago

Says the guy upvoting himself.

i made some good points i think

tokengamedev commented 1 month ago

Ignoring the trolling @scotmcp it seems in the mentioned issue in Godot repository https://github.com/godotengine/godot/issues/95461 is going to be addressing the issue. This issue will require design change. It may take a bit of time as it may impact multiple scenarios as by definition null is being used to unset properties in resources. this may break some compatibility too. In case they are able to address the issue in 4.4 as they say, it can be pulled over here.

SkogiB commented 1 month ago

When there are issues immediately presented to Godot, we're going to give their team time to work on it, because sometimes they do. If we make a fix and merge it, then THEY make a fix for themselves, it creates huge merge problems on our end. Give the Godot crew some time and then if the issue never gets a PR or if a PR languishes for too long, we'll look at porting it over.

tokengamedev commented 1 month ago

@jawbroken, FYI, it can be fixed easily, but the repercussion is huge on the engine, as there are lot of places this may be used, apart from already developed games using it as a feature. I commend the review process and due diligence Godot contributors take in their development; hope they can take it up or find a workaround. @SkogiB, I may take a jab at it in the upcoming week to try for a refactor, without causing much of merge issues.

dandykong commented 1 month ago

@jawbroken And when the "real project" permanently blocks you for your off-platform beliefs? What then?

SkogiB commented 1 month ago

@jawbroken, FYI, it can be fixed easily, but the repercussion is huge on the engine, as there are lot of places this may be used, apart from already developed games using it as a feature. I commend the review process and due diligence Godot contributors take in their development; hope they can take it up or find a workaround. @SkogiB, I may take a jab at it in the upcoming week to try for a refactor, without causing much of merge issues.

Right on, it'll be good to maybe have a fix sitting in the chamber we can pull the trigger on if they never want to touch it. It sounds like at least one guy over there would like to improve imports for 4.4 to fix this so we'll see what they work on.