Skip to content
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

Add Images::getScalarData(std::uint32_t imageIndex, ...) overload for retrieving image scalar data in one operation #629

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

ThomasKroes
Copy link
Contributor

No description provided.

@ThomasKroes ThomasKroes added the enhancement New feature or request label Jun 28, 2024
@ThomasKroes ThomasKroes self-assigned this Jun 28, 2024
Copy link
Contributor

@bldrvnlw bldrvnlw left a comment

Choose a reason for hiding this comment

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

getScalarData is overloaded with a vector of dimensionIndices which can be used to retrieve all the colour components which is not done in the original getScalarData . However this function name and signature (and the documentation) doesn't really make it clear that colour components. Or maybe I'm missing something...

Should the name of the comments be changed? I'm open to being convinced otherwise.

@ThomasKroes
Copy link
Contributor Author

However this function name and signature (and the documentation) doesn't really make it clear that colour components.
What do you mean?

Did you notice the addition of void Images::getImageScalarData(...)?

@bldrvnlw
Copy link
Contributor

Yes I noticed void Images::getImageScalarData(...) and it uses this new getScalarData overload. I'm happy with that function.

But my comment is about the getScalarData overload itself. That does something that I think is radically different from the original version because of the way image components are handled. In my opinion this isn't clear because it is an overload so the name is the same and there is no mention of special component handling in the comments.

But am I either over-complicating or misinterpreting things?

Copy link
Contributor

@bldrvnlw bldrvnlw left a comment

Choose a reason for hiding this comment

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

Image retrieval tested with JupyterPlugin

@ThomasKroes ThomasKroes merged commit b6cb5e9 into master Jul 15, 2024
8 checks passed
@ThomasKroes ThomasKroes deleted the feature/add_images_get_scalar_data_overloaded branch July 15, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Images::getScalarData() overload that returns an entire image (with all its components)
2 participants