Add basic SpotLight instancing by JezSonic · Pull Request #249 · MaSzyna-Reloaded/MaSzyna-API-wrapper

JezSonic · 2026-05-12T20:31:21Z

Fixes #134

marcinn · 2026-05-12T21:40:03Z

+    if parent == null:
+        return
+    var light_root = parent.get_parent()
+    var is_end = parent.name.begins_with("end")


I'm not sure about this parent-child relation and hardcoded names of related nodes.

Me neither, but that's the best I could come up with. Also _end is a part of their standard naming of light-related nodes

I think that e3d_light contains logic which should be moved to e3d instancing or e3d parsing stage.

MaSzyna quirks like matching something by part of a node name should be configurable. Please note that node name in Godot can be different than e3d (sub)model name.

E3D Light in this implementation will be always a spotlight. You never be able to create omni lights or other lights. That's why it is better to move nodes creation to the factories like e3d nodes instancer.

At the e3d parsing stage you can traverse e3d model tree to find parent-child relationships between submodels, if needed.

Try to store references between e3d submodels in thei, i.e. add properties similar to nodepath (you can't use nodepath though, but a reference to other node should work)

marcinn · 2026-05-12T21:44:46Z

+func _ready():
+    _update_state()
+
+func _update_state():


I think the high level API for lights control should be on E3DModelInstance, which is a node proxy for instancer and a reference holder for a e3d model.

Optimised instancer will not use this node. It will control lights by calling RenderingServer directly.

marcinn · 2026-05-12T21:47:13Z

+    for light in lights:
+        var base_name = light.name.to_lower()
+
+        var on_suffixes = ["_on", "_xon"]


Instancer should work with a structure resolved by parser. E3dmodel should have references to light nodes. We shouldn't implement e3d quirks here like finding node by specific name

marcinn · 2026-05-12T21:48:43Z


+        E3DSubModel.SubModelType.SUBMODEL_FREE_SPOTLIGHT:
+            obj = SpotLight3D.new()
+            obj.set_script(preload("res://addons/libmaszyna/e3d/e3d_light.gd"))


It was generated by AI, right?

So I don't understand why we're using set_script and preloading instead of classes like MyCustomClass.new()

marcinn · 2026-05-12T21:49:26Z

+		<member name="light_range" type="float" setter="set_light_range" getter="get_light_range" default="0.0">
+			The maximum range of the light source.
+		</member>
+		<member name="near_atten_start" type="float" setter="set_near_atten_start" getter="get_near_atten_start" default="0.0">


Please use full names instead of shortcuts

marcinn · 2026-05-12T21:55:30Z

It's almost fine. I would like to move lights controller to e3dmodelinstance, which will delegate lights handling to a selected instancer. E3DSpotlight node is probably unnecessary.

I think the code is partially AI assisted. Please clean the code before next review (inconsistent naming, preloading scripts inside the logic code, using set_script etc)

marcinn

Stil, I think that references to lights / array of lights should be implemented at the resource layer (e3dmodel/e3dsubmodel). Instancer should only fetch the lights and apply transforms if needed.

Also there should be a kind of high level API for toggling lights - E3DModelInstance should be a good place, because it is a connector between e3d model and instancer. E3DModelInstance should delegate light control to instancer, same as instantiating and cleaning up.

This PR is not handling texture based lights. We can split this, of course, but thinking about submodels with emissive materials as lights (instead of spotlights) should be helpful for designing the lights api.

marcinn · 2026-05-17T16:50:49Z


+        E3DSubModel.SubModelType.SUBMODEL_FREE_SPOTLIGHT:
+            obj = SpotLight3D.new()
+            obj.set_script(preload("res://addons/libmaszyna/e3d/e3d_light.gd"))


So I don't understand why we're using set_script and preloading instead of classes like MyCustomClass.new()

marcinn · 2026-05-17T16:51:23Z

+    if parent == null:
+        return
+    var light_root = parent.get_parent()
+    var is_end = parent.name.begins_with("end")


I think that e3d_light contains logic which should be moved to e3d instancing or e3d parsing stage.

MaSzyna quirks like matching something by part of a node name should be configurable. Please note that node name in Godot can be different than e3d (sub)model name.

E3D Light in this implementation will be always a spotlight. You never be able to create omni lights or other lights. That's why it is better to move nodes creation to the factories like e3d nodes instancer.

At the e3d parsing stage you can traverse e3d model tree to find parent-child relationships between submodels, if needed.

Try to store references between e3d submodels in thei, i.e. add properties similar to nodepath (you can't use nodepath though, but a reference to other node should work)

marcinn · 2026-05-17T16:52:54Z

+            if not child:
+                continue
+
+            if child.has_method("_update_state"):


this is not a property which identifies the light. you're assuming here that any object with "_update_state" is a light.

marcinn · 2026-05-17T16:57:33Z

            if child is Node3D and submodel.transform:
                var child_node:Node3D = child as Node3D
-                child_node.transform = submodel.transform
+                if child is SpotLight3D:


This code assumes that there is only one type of light - a SpotLight3D, not even E3DLight. Checking E3DSubModel.SubModelType.SUBMODEL_FREE_SPOTLIGHT will be consistent with _create_submodel_instance() impl.

…elfillum)

JezSonic self-assigned this May 12, 2026

JezSonic force-pushed the dev/e3d-model-lights branch from 50396df to a712969 Compare May 12, 2026 20:32

JezSonic requested a review from marcinn May 12, 2026 20:33

JezSonic force-pushed the dev/e3d-model-lights branch from a712969 to 79bf9a6 Compare May 12, 2026 20:39

marcinn requested changes May 12, 2026

View reviewed changes

marcinn requested changes May 17, 2026

View reviewed changes

JezSonic force-pushed the dev/e3d-model-lights branch 5 times, most recently from f863cac to b197cc2 Compare May 24, 2026 21:28

JezSonic changed the title ~~Add basic SopotLight instancing~~ Add basic SpotLight instancing May 24, 2026

JezSonic and others added 6 commits May 25, 2026 19:29

Add basic SpotLight instancing

0783382

instantiate lights and toggle via E3DModelInstance.lights_state

30d30ce

little fixes/tuning by guess

2d87b5d

Refactor MaterialManager to support more options (like e3d submodel s…

cf2a1ea

…elfillum)

add emission

9201da5

initial high level lights api on railvehicle3d

175af9e

marcinn force-pushed the dev/e3d-model-lights branch from 193e997 to 175af9e Compare May 25, 2026 17:29

Improve detecting light_energy. Add ep09 for tests

0491f08

Conversation

JezSonic commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcinn commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcinn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marcinn commented May 12, 2026 •

edited

Loading