Merge Reconciliation#142
Conversation
|
Initial pass through for node merging logic. Implemented simple LatestAttribute merge logic. Currently we "merge" current object attributes and new attributes. Future we want to track all previous proposed attributes but the current logic should reproduce existing behaviors. |
nathanhhughes
left a comment
nathanhhughes
left a comment
There was a problem hiding this comment.
This is on the right track / is roughly what I was thinking! We should talk about the logic for updateNode if my comment isn't clear (and I can talk you through how to implement the logic for how to maintain the set of merges and cached attributes which can be tricky to get right the first time you do it)
|
|
||
| struct AttributeMerger { | ||
| virtual ~AttributeMerger() = default; | ||
| virtual std::unique_ptr<spark_dsg::NodeAttributes> merge( |
There was a problem hiding this comment.
(minor) this is better as spark_dsg::NodeAttributes::Ptr. Other places in the code have to be explicit to preserve the inner type of the unique pointer, e.g., std::unique_ptr<KhronosObjectAttributes> is different than KhronosObjectAttributes::Ptr (unless Lukas overrode the base class typedef with a derived variant)
|
|
||
| } // namespace | ||
|
|
||
| std::unique_ptr<spark_dsg::NodeAttributes> LatestAttributeMerger::merge( |
There was a problem hiding this comment.
(minor) okay to do using spark_dsg::NodeAttributes> somewhere in this file, avoiding these line breaks would be good
| const std::vector<const spark_dsg::NodeAttributes*>& attrs) const = 0; | ||
| }; | ||
|
|
||
| struct LatestAttributeMerger : public AttributeMerger { |
There was a problem hiding this comment.
Technically we would want an "Earliest" variant as well (which I think you mentioned in slack but might not have implemented), that actually matches the current behavior without these merge functors
| char prefix = 0; | ||
| std::optional<spark_dsg::LayerId> target_layer; | ||
| config::VirtualConfig<NodeMatcher> matcher; | ||
| config::VirtualConfig<AttributeMerger> merger; |
There was a problem hiding this comment.
I think it would help to clean up the implementation of addNode and updateNode to actually require that a merger is specified and that it defaults to EarliestAttributeMerger::Config to match the current behavior
| if (tracker.merger) { | ||
| const auto* existing = graph.findNode(map_iter->second); | ||
| if (existing) { | ||
| updated = | ||
| tracker.merger->merge({&existing->attributes(), entry.attributes.get()}); | ||
| } | ||
| } |
There was a problem hiding this comment.
We should talk about this implementation, this is where things actually get tricky. For any node that is updated, we should always take the updated attributes over the previous ones (in some attribute cache local to the graph updater). After that, we look to see if the updated node id was involved in any merge and then (i) check if the merge is still valid for all the involved nodes given the updated attributes and (ii) recompute the merged attributes given all the nodes involved in the merge and add them to the scene graph
nathanhhughes
left a comment
nathanhhughes
left a comment
There was a problem hiding this comment.
LGTM, thanks! Let's merge this in and we can spend more time on the actual logic later as it makes sense
2 participants
No description provided.