Accept more valid clusters for coloring - #253
Merged
Merged
Conversation
alxvth
commented
Aug 17, 2026
alxvth
commented
Contributor
- Remove restrictive condition added in Fix loading colors from cluster in minimized scatterplot in project #216 - we do not need to guard the standard behavior
- Check if cluster indices exceed point indices instead of checking it they provide full coverage - this also allows to re-color with with more cluster data sets
ThomasKroes
approved these changes
Aug 18, 2026
ThomasKroes
left a comment
ThomasKroes
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me overall, thanks! 👍
One small consideration regarding _numTotalPoints: although the current approach should work, I am slightly hesitant about caching the total number of points as additional state in the plugin.
My preference would be to retrieve the current number of points directly from the position/source dataset whenever we need it. That way the dataset remains the single source of truth, and we don't introduce the possibility of _numTotalPoints becoming stale if the underlying dataset changes through a code path that does not trigger positionDatasetChanged().
Since retrieving getNumPoints() should be essentially free, I think avoiding the cached state may make this a little more robust in the long run.
I don't consider this a blocker for the PR, so approving it as-is, but I would be in favor of changing this if you agree.
Contributor
Author
|
Sure, we can do a |
Contributor
Great! |
Contributor
Author
|
Almost forgot: gcc uses TBB for |
alxvth
force-pushed
the
feature/FixClusterAccept
branch
from
August 18, 2026 09:43
dee7f3c to
fa4d860
Compare
ThomasKroes
added a commit
that referenced
this pull request
Aug 18, 2026
* Fix warnings and update points (#240) * Update number of points to uint64 * Use reference dataset * Add some const * Rename lambda capture variable to not shadow function paramters * More uint64 * Set MSVC warning level to W3 * We only want one dataset * Update core requirement due to previous commit * Adhere to new serialization API (#243) * Use new getter for clarity (avoid negation) (#242) * Use `mv_project_defaults()` for setting CMake defaults (#241) * Use mv project defaults * Simplify unity build setup * Prefer target based properties * Set cache variable instead of normal variable for CMake option * Adhere to revamped core --------- Co-authored-by: Alexander Vieth <a.vieth@tudelft.nl> * Set current point dataset when opacity dataset changed * Add extra null guard * Extends coloring options for scatterplot (addressing issue #24) (#247) Adds 2D and 3D coloring options. 2D allows arbitrary 2 channels using the build in 2D colormaps 3D allows arbitrary 3 channels mapping directly to RGB (normalized in shader) Modes are automatically picked when datasets with exactly 2 or 3 channels are set as color or can be manually set using the extended color action Renames 2D colormaps according to their authors * Fixes Qt 6.10 build Replaced deprecated 'mirrored' method with 'flipped' for color maps. * CI: Remove Release build and install steps (#248) * Upgrade build workflow actions and Python version Updated build workflow to use newer versions of actions and Python. * Revert principal dimension action name change (#250) * Revert principle dimension action name change * Ignore loading errors for newly introduced actions Do this for backwards compatibility * Remove restrictive condition * Use range for, eliminates index * Track totalPoints class wide * Check if cluster indices exceed point indices instead of checking of they provide full coverage * Revert last 4 commits * Accept more valid clusters for coloring (#253) * Remove restrictive condition * Use range for, eliminates index * Check if cluster indices exceed point indices instead of checking of they provide full coverage * link against tbb with gcc --------- Co-authored-by: Alexander Vieth <a.vieth@tudelft.nl> Co-authored-by: Julian Thijssen <julianthijssen@gmail.com> Co-authored-by: Soumyadeep Basu <44787782+sbvis@users.noreply.github.com> Co-authored-by: Thomas Höllt <thoellt@me.com> Co-authored-by: Alexander Vieth <a.vieth@lumc.nl> Co-authored-by: Alexander Vieth <vieth.alexander@gmx.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.