Skip to content

fix(AbstractMapper): Cleanup mapper classes #2186

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 1 commit into from
Jan 10, 2022

Conversation

DrewLazzeriKitware
Copy link
Contributor

These changes fix problems with Mapper classes found during #2113.
Specifically:

  • The class hierarchy for AbstractMapper was missing
  • VolumeMapper was calling macro.obj and macro.algo (once through parent)
  • AbstractMapper.removeClippingPlane was using indexes instead of instances
  • AbstractMapper.removeClippingPlane was checking classes too specifically
  • AbstractMapper methods were not calling modified.

BREAKING CHANGE:
Changed removeClippingPlane to use instance instead of index.

@DrewLazzeriKitware
Copy link
Contributor Author

@finetjul I've split the AbstractMapper improvements from the MPR #2113 so they can be merged more easily.

Please review.

@jourdain
Copy link
Collaborator

jourdain commented Dec 2, 2021

LGTM

FYI @floryst @agirault @ebrahimebrahim

@@ -42,7 +42,7 @@ export interface vtkAbstractMapper extends vtkAbstractMapperBase {
* Remove clipping plane at index i.
* @param {Number} i
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also fix the comment and the params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks good catch. Fixed.

@floryst
Copy link
Collaborator

floryst commented Dec 3, 2021

@DrewLazzeriKitware your AbstractMapper changes should be the desired one in resolving the conflict.

Added missing class hierarchy for AbstractMapper.
Removed extra extend calls from VolumeMapper.

BREAKING CHANGE:
Changed removeClippingPlane to use instance instead of index.
@DrewLazzeriKitware
Copy link
Contributor Author

@floryst The merge conflict is resolved

@finetjul
Copy link
Member

finetjul commented Dec 5, 2021

LGTM

Copy link
Collaborator

@daker daker left a comment

Choose a reason for hiding this comment

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

The typescript part is good for me.

@jourdain
Copy link
Collaborator

jourdain commented Dec 7, 2021

@floryst since it is a BREAKING_CHANGE, I let you merge it when you see fit.

@floryst floryst mentioned this pull request Jan 10, 2022
5 tasks
@floryst floryst merged commit 63158bf into Kitware:master Jan 10, 2022
@github-actions
Copy link

🎉 This PR is included in version 22.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Automated label label Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Automated label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants