Group optical properties#144
Conversation
There was a problem hiding this comment.
Pull request overview
Refactor that hoists per-element OpticalProperties into a shared OpticalPropertySet registry (SimulationData::add_optical_property_set) and replaces front/back getters on Element with a single optics_id reference. The goal is to deduplicate optics in serialized JSON. The change is broad: data model, all CST templates, native/embree/optix runners, JSON I/O, and ~20 tests.
Changes:
- New
OpticalPropertiesFace(per-face) +OpticalPropertySet(per-element) split, withoptics_idreferences and aContainer-backed registry onSimulationData(including a built-inOPTICS_ID_VIRTUAL). - Element/CST API switched from
set_front/back_optical_propertiestoset_optical_property_set_id(andset_optics(id, …)variants on composite templates), with runners plumbingLastHitBackSidethrough interaction/error code so per-face data is selected at trace time. - JSON read/write now serializes a separate
optical_propertiessection and storesopt_idon each element;.stinputloader builds and dedupes sets viafind_or_add_optical_property_set.
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| coretrace/simulation_data/optical_properties.{hpp,cpp} | Split into OpticalPropertiesFace + OpticalPropertySet, add comparison/IO helpers |
| coretrace/simulation_data/simulation_data.{hpp,cpp} | Add OpticalPropertySet registry, built-in virtual entry, find/get accessors |
| coretrace/simulation_data/container.hpp | Add reset, insert_item, recompute_next_id for JSON load support |
| coretrace/simulation_data/single_element.{hpp,cpp} | Drop in-element optics, store optics_id, enforce-on-validate |
| coretrace/simulation_data/composite_element.hpp / element.hpp | Replace per-face virtuals with id getter/setter |
| coretrace/simulation_data/virtual_element.{hpp,cpp} | Bind virtual element to OPTICS_ID_VIRTUAL |
| coretrace/simulation_data/simulation_parameters.hpp | Add JSON ctor / write_json |
| coretrace/simulation_data/simdata_io.cpp | New JSON serialization for optics; rework .stinput to populate registry |
| coretrace/simulation_data/cst_templates/{parabolic_trough,parabolic_dish,linear_fresnel,heliostat}.{hpp,cpp} | Switch set_optics to take ids; remove embedded OpticalProperties members |
| coretrace/simulation_runner/native_runner/{trace,process_interaction,tracing_errors,determine_interaction_type,native_runner,native_runner_types}.{cpp,hpp} | Carry OpticalPropertySet* + LastHitBackSide through tracing pipeline |
| coretrace/simulation_runner/embree_runner/trace_embree.cpp | Mirror native runner changes |
| coretrace/simulation_runner/optix_runner/optix_runner.cpp | Resolve optics via SimulationData lookup |
| coretrace/simdata_bridge.cpp | Bridge legacy TOpticalProperties into new set/id |
| google-tests/**/* | Migrate tests to new API (some set_optics cases left commented out) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| virtual void set_optical_property_set_id(optics_id) override | ||
| { | ||
| return nullptr; | ||
| assert(false && "CompositeElement does not support optical property sets"); | ||
| } |
There was a problem hiding this comment.
@taylorbrown75 I don't think we even need the assert here. The reasoning is that the composite element is really just a container for other elements (really it is just an intermediate coordinate system) and it never has optical properties. If these could in some way have effects during ray tracing, then it should probably be an error (i.e. throw something).
But it can't. These things never interact with rays except through coordinate transformations so the optical properties can't do anything. Even if a user sets these properties, it will have no impact on anything, and can not lead to any unintended consequences. So I would say don't even bother with the assert. The function should just be a no-op.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Replace raw optics_id usage with OpticalPropertySetReference (id + weak_ptr) and update APIs to pass/resolve optical property sets by reference. Added OpticalPropertySetReference and OpticalPropertySetResolver; SimulationData::add/find now return references. Element interface changed to set/get optical property set via OpticalPropertySetReference/shared_ptr; SingleElement stores a weak_ptr and enforces presence of a valid set. Composite/Stage constructors and JSON loading now accept a resolver to resolve optics during construction. VirtualElement now owns a permanent OpticalPropertySet for virtual elements. Updated CST templates, runners, IO, and unit tests to use the new reference-based optics API to avoid sentinel id lookup and enable shared ownership/direct access to optical property sets.
jmaack24
left a comment
jmaack24
left a comment
There was a problem hiding this comment.
This looks good to me! Good work!
|
json_test_1.json |
|
@taylorbrown75 @qualand When did we want to merge this? I recall that there was some discussion of waiting but I don't remember what the conclusion was. |
Only reason to wait is if GUI team uses JSON and we want to merge their PR before this one. UW is aware of this PR. |
Change structure of optical properties so they are grouped in SimulationData, and each element has an identifier pointing to a specific value in that collection. Previously, elements stored their complete optical properties, even if they were shared by many elements. This caused the JSON to become very large.