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

Generalize up direction #1536

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

snoyer
Copy link
Contributor

@snoyer snoyer commented Jul 14, 2024

would fix #1533

@snoyer snoyer marked this pull request as ready for review July 14, 2024 10:07
Copy link

codecov bot commented Jul 14, 2024

Codecov Report

Attention: Patch coverage is 96.24060% with 5 lines in your changes missing coverage. Please review.

Project coverage is 96.78%. Comparing base (6701eeb) to head (c2e5b47).

Files Patch % Lines
vtkext/private/module/vtkF3DRenderer.cxx 95.93% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1536      +/-   ##
==========================================
- Coverage   96.81%   96.78%   -0.03%     
==========================================
  Files         106      106              
  Lines        7823     7907      +84     
==========================================
+ Hits         7574     7653      +79     
- Misses        249      254       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 1740 to 1744
const std::string repr = std::string(prop3d->GetClassName());
F3DLog::Print(F3DLog::Severity::Warning,
"Could not properly account for " + repr +
" in non-axis-aligned bounds computation");
extendBoxAxisAligned(prop3d, box);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a fallback in the unlikely case there is no custom bounds implementation for a given prop3d. It should not happen if we have all the possible props/actors/mappers types handled so I don't think it can be covered by tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

then let's assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't a plugin use some exotic prop we haven't covered tho?

Copy link
Contributor

@Meakk Meakk left a comment

Choose a reason for hiding this comment

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

here are some comments

testing/baselines/TestGridX.png Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Show resolved Hide resolved
Comment on lines 1740 to 1744
const std::string repr = std::string(prop3d->GetClassName());
F3DLog::Print(F3DLog::Severity::Warning,
"Could not properly account for " + repr +
" in non-axis-aligned bounds computation");
extendBoxAxisAligned(prop3d, box);
Copy link
Contributor

Choose a reason for hiding this comment

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

then let's assert?

vtkext/private/module/vtkF3DRenderer.cxx Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DRenderer.cxx Show resolved Hide resolved
double delta[3];
this->GetEnvironmentUp(delta);
vtkMath::MultiplyScalar(delta, downShift);
vtkMath::Subtract(gridPos, delta, gridPos);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure here, but it seems like you are using the 4x4 matrix to rotate and then this code does the translation.
You can encode rotation and translation within the same 4x4 matrix (last column for the translation)


double orientation[3];
vtkTransform::GetOrientation(orientation, upMatrixInv);
this->GridActor->SetOrientation(orientation);
Copy link
Contributor

Choose a reason for hiding this comment

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

If properly built, you can just use SetUserMatrix directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize "UP" vector to be any direction instead of axis aligned
2 participants