GodotVR / godot-xr-tools

Support scenes for AR and VR in Godot
MIT License
471 stars 64 forks source link

Viewport2Din3D: Forward properties of supplied scene object #637

Closed NetroScript closed 2 weeks ago

NetroScript commented 1 month ago

This PR adjusts the viewport_2d_in_3d.gd script to allow forwarding / proxying of the scene provided as PackedScene.

Currently, to assign properties to the scene shown in the Viewport, it would be necessary to enable editable children, instance the scene as child, and assign properties to that node.

This however adds noise to the SceneTree, which could be reduced if these properties could be directly editable without needing the children in the tree.


The script works by reading all properties of the script of the packaged scene, and forwarding the property accesses (reading and writing) to the object itself it it exists. On scene load before it is instantiated at least once, a proxy object is used, where then the values are applied to the instanced scene as soon as it loads.

To get properties Script.get_script_property_list() is currently employed, this means only custom defined exports of the script are displayed (not inherited properties).

Example Image:

Screenshot_20240524_140514

Thoughts

It might be worth a consideration if one wants to extend this to the current class if no script is attached to the scene (using ClassDB.class_get_property_list(class, true)) or if even all editable properties of the underlying object should be exposed. (Although then it would probably better to create a dummy Resource which contains all of those properties to have less clutter and a clear separation between the current node, and all properties of the embedded node).

Potential Issues

Currently if a property has the same name in the provided scene and the Viewport2Din3D script, the property assignment will always be done to the provided scene, and it won't be possible to access this value for the Viewport2Din3D script anymore.

As these values are applied to the parent (which then forwards it to the scene) for the Editor, all relative things might need special handling. This PR already handles this for NodePath by adjusting the value when getting / setting by prepending / removing ../../.

Malcolmnixon commented 2 weeks ago

This is looking quite good; but there are some gdlint issues being reported in the new code:

.\godot-xr-tools\objects\viewport_2d_in_3d.gd:221: Error: Max allowed line length (100) exceeded (max-line-length)
.\godot-xr-tools\objects\viewport_2d_in_3d.gd:274: Error: Max allowed line length (100) exceeded (max-line-length)
.\godot-xr-tools\objects\viewport_2d_in_3d.gd:115: Error: Trailing whitespace(s) (trailing-whitespace)
.\godot-xr-tools\objects\viewport_2d_in_3d.gd:170: Error: Trailing whitespace(s) (trailing-whitespace)
.\godot-xr-tools\objects\viewport_2d_in_3d.gd:180: Error: Trailing whitespace(s) (trailing-whitespace)
.\godot-xr-tools\objects\viewport_2d_in_3d.gd:202: Error: Trailing whitespace(s) (trailing-whitespace)
.\godot-xr-tools\objects\viewport_2d_in_3d.gd:215: Error: Trailing whitespace(s) (trailing-whitespace)
.\godot-xr-tools\objects\viewport_2d_in_3d.gd:264: Error: Trailing whitespace(s) (trailing-whitespace)
.\godot-xr-tools\objects\viewport_2d_in_3d.gd:270: Error: Private method "_get_property_list" has been called (private-method-call)

Most of them are simple formatting; but you may want to investigate calling the public get_property_list() method exposed by all godot Objects rather than the private _get_property_list().

NetroScript commented 2 weeks ago

Fixed the linting issues.