Skip to content

Override visual object colors with tags #4920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Aug 5, 2025
Merged

Conversation

oitel
Copy link
Contributor

@oitel oitel commented Aug 4, 2025

No description provided.

@oitel oitel requested a review from Grantim August 4, 2025 12:05
Comment on lines 255 to 259
/// provides read-only access to the metadata storage
/// the storage is a set of unique strings
const std::unordered_set<std::string>& getMetadata() const { return metadata_; }
/// provides read-write access to the metadata storage
std::unordered_set<std::string>& getMutableMetadata() { return metadata_; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can both functions be named just tags() ? (also we usually virtualize non-const getters) (for models we have mesh() and varMesh() for example)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also it seems that set and update functions are required (to set needRedraw_ flag internally)

@@ -216,6 +216,8 @@ class MRVOXELS_CLASS ObjectVoxels : public ObjectMeshHolder
/// nullptr means serialize in defaultSerializeVoxelsFormat()
MRVOXELS_API void setSerializeFormat( const char * newFormat );

MRVOXELS_API void resetFrontColor() override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add comments

Comment on lines 255 to 259
/// provides read-only access to the tag storage
/// the storage is a set of unique strings
const std::unordered_set<std::string>& getTags() const { return tags_; }
/// provides read-write access to the tag storage
std::unordered_set<std::string>& getMutableTags() { return tags_; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getTags -> tags
getMutableTags -> virtual varTags (to be more like existing functions)

Comment on lines 567 to 570
void ObjectVoxels::resetFrontColor()
{
setDefaultColors_();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add comment in all such places, that we cannot do

setDefaultColors_
{
resetColors();
}

to keep setDefaultColors_ non-virtual and resetColors - virtual

/// add the tag to the object and apply related visual object properties
MRVIEWER_API static void applyTag( VisualObject& visObj, const std::string& visTagId, bool force = false );
/// remove the tag from the object and reset the visual object properties
MRVIEWER_API static void revertTag( VisualObject& visObj, const std::string& visTagId, bool force = false );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert->remove?

Comment on lines 43 to 44
/// add the tag to the object and apply related visual object properties
MRVIEWER_API static void applyTag( VisualObject& visObj, const std::string& visTagId, bool force = false );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply->add?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mb we should separate add/remove/apply:
for example create add/removeTag functions to MR::Object and call apply on these function via signals or tagApplyCallback registry or something?

Comment on lines +47 to +48
/// update visual object properties according to whether given object has the visual object tag or not
MRVIEWER_API static void update( VisualObject& visObj, const std::string& visTagId );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be able to update all tags present in storage at once?

@oitel oitel merged commit 9ad188b into master Aug 5, 2025
25 checks passed
@oitel oitel deleted the feature/visual_object_tags branch August 5, 2025 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants