search

GodotVR / godot-xr-tools

Support scenes for AR and VR in Godot
MIT License
504 stars 74 forks source link

Fix issue629 #630

Closed esodan closed 1 week ago

esodan commented 6 months ago

This should be merged only if #627 is fixed first, as this depends on it

Malcolmnixon commented 6 months ago

The hand change looks good, but if you want to do any changes to staging; could you do that in a separate PR.

Ah, i see - yes dependent changes. Never mind.

BastiaanOlij commented 6 months ago

It would be good to have more of an explanation in your description on why you are changing this.

The staging system is meant to be used with the staging scene, which is a scene you can inherit if there is more that you want to add in. This just introduces more work for a user to set things up that should already be there.

So without knowing the justification for this change, it breaks the "keep things simple" rule.

esodan commented 6 months ago

It would be good to have more of an explanation in your description on why you are changing this.

The staging system is meant to be used with the staging scene, which is a scene you can inherit if there is more that you want to add in. This just introduces more work for a user to set things up that should already be there.

So without knowing the justification for this change, it breaks the "keep things simple" rule.

The comunity provides lot of tutorials, most of them helps you to "walk" for each step, adding by hand each node. This is good to know better how to make your own stuff.

After following that tutorials, make lot of mistakes and found this missing, underdocumented things like Fade. So if the user wants to do it by hand, this should be better to be added as a warning. Yes there are more steps here, but that is the price to do it by hand.

May is better to document, that the user should instantiate the staging scene, instead to create all step by step.

BastiaanOlij commented 1 week ago

Discussing this further with @Malcolmnixon , we need to double check the hands fix and make sure that was applied.

As for the changes to staging, the staging.gd script is part of the staging.tscn file which contains all relevant nodes. This looks like an issue with using the script without the scene, which is something Godot allows while it shouldn't.

This needs better tutorials around the staging system but not code changes.

Closing this PR.