feat(rm): allow registering custom radio mat. prior to loading scenes - #71
feat(rm): allow registering custom radio mat. prior to loading scenes#71jeertmans wants to merge 4 commits into
Conversation
|
Hello @jeertmans, Thank you for this PR, it looks quite useful! We will review it as well as #70. |
| registered_rm.color = mi_bsdf.color | ||
| registered_rm.thickness = mi_bsdf.thickness | ||
| elif hasattr(mi_bsdf, "properties"): | ||
| props = mi_bsdf.properties() | ||
| for pname in ("color", "reflectance", "base_color"): | ||
| if props.has_property(pname): | ||
| registered_rm.color = tuple(props[pname]) | ||
| break | ||
| if props.has_property("thickness"): | ||
| registered_rm.thickness = float(props["thickness"]) |
There was a problem hiding this comment.
I feel like this logic is a bit risky: now any BSDF attached to a shape in the current scene can overwrite properties inside of the radio_material_registry, including for the next shapes in the same scene and future loaded scenes.
It's especially tricky because for ITU materials, "Multiple Material Customizations Referencing the Same ITU Material Type" is explicitly allowed.
IMO, it makes more sense to consider the BSDFs in the registry as immutable and throw an error if an unsupported customization is attempted.
There was a problem hiding this comment.
I see! I like having the ability to overwrite some parameters, like colors, directly in the XML file. Otherwise, the parameters could only be set via the registration utilities. However, I agree that this could cause problems if a material is suddenly updated when reading a scene file.
Here are some solutions I have thought of. Let me know which one you prefer, or if you prefer something else.
- (A) Disallow specifying new RM in the XML file and raise an error if any of the properties differ;
- (B) Same as (A), but raise a warning instead (and do not overwrite parameters, just let the user know);
- (C) Allow overwriting radio material properties, but how would you implement this?
- (D) Same as (C), but do not raise an error if the parameters are set to the same values (just ignore it) and report to the user when the values are different.
It depends on what you mean by
unsupported customization is attempted
:-)
I ended up implementing a version of (C), but scoped by property rather than all-or-nothing: color is purely cosmetic (only read by the renderer/previewer, never touches ray tracing), so it's safe to override per-shape: I do this by returning an independent clone (RadioMaterial.with_color()) rather than mutating the registered instance, so the registry and any other shape using it are untouched. Anything else (thickness, etc.) now raises a clear error telling you to set it on the registered material directly, since silently letting that vary per-shape is exactly the invisible-global-state problem you flagged. Pushed as 0c2d1de, but this required quite some lines of code and possibly breaking changes if you consider process_xml to be public API; let me know what you think.
There was a problem hiding this comment.
Thanks for giving it a shot!
(A) would be the easiest option but also the most restrictive.
(C) complicates the implementation quite a lot as you saw, but is the most convenient & flexible.
As I think that you noticed, if we go with (C), it is critical for performance that RadioMaterial instances that share identical properties must be referenced in all of the shapes that use it, rather than having one individual copy for each shape. You must also make sure that the merge_shape mechanism still functions as expected when introducing custom radio materials with / without overrides.
In the current implementation, since you introduced a method to clone a radio material, why only allow overriding colors? If the cloning method is correct, then it should be fine to override any property, no?
There was a problem hiding this comment.
That's a fair point: I actually avoided mutating differentiable attributes, thinking it could cause surprises when trying to optimize a radio material property, and then observe that, because a material a been clone multiple times, the property doesn't get updated on all the clones. However, after checking again, the problem was already present with my solution. So, I went for a more general "clone" solution, but with some documentation about how one just use it when performing differentiation.
I also decided to add a logging message to inform the user about material cloning when loading a scene file, as I believe this could be the main source of "surprises". I set the level to INFO, but let me know if that should be changed. I think Sionna RT is not using a logger anywhere else in its codebase, so I don't know what your position on this.
0c2d1de to
41a20bb
Compare
41a20bb to
94507b8
Compare
Hi!
While this PR is linked to #70, it does not depend on #70 and should not cause any conflicts.
Currently, loading a scene file containing unknown radio material names or unlisted ITU radio materials raises an error. This presents a challenge in two common scenarios:
Currently, custom radio materials can only be assigned after loading the scene, which I believe can be error-prone and time-consuming for large scenes.
This PR introduces two helper functions,
register_itu_radio_materialandregister_radio_material, to make registering custom radio materials idiomatic before callingload_scene.Summary of changes:
register_itu_radio_material: Addsregister_itu_radio_material(name, parameters, color=None)to allow registering custom ITU materials or updating existing parameters and visual colors inITU_MATERIALS_PROPERTIESandITU_MATERIAL_COLORS. Ifcoloris omitted or set toNone, a random color is generated upon material instantiation, matchingRadioMaterial. This changes the previous behavior, which required a color even though it could eventually be overridden.register_radio_material: Addsregister_radio_material(rm)andradio_material_registryto registerRadioMaterialBaseinstances prior to scene loading.process_xmlandScene._load_scene_objectsto handle both explicit XML plugin syntax (<bsdf type="itu-radio-material"...>and<bsdf type="radio-material"...>) as well as legacy/Blender syntax (id="itu_..."orid="mat-itu_..."). Any attribute specified in the XML file (such asthicknessandcolor) correctly overrides default or pre-registered material attributes.my_custom_thick_woodandmy_custom_thin_wood) referencing the same underlying ITU material type while specifying different property overrides (e.g. thickness).doc/source/api/radio_materials.rstand comprehensive developer guide examples indoc/source/developer/dev_custom_radio_materials.rstdemonstrating explicit XML syntax, material reuse, and attribute overrides.test08_register_itu_radio_materialandtest09_register_radio_materialintest_scene_utils.pycovering prefix error validation, explicit vs legacy XML syntax, optional color generation, multiple material IDs referencing the same ITU type, and XML thickness/color overrides.Let me know if you'd like any adjustments! :-)