Skip to content

Conversation

@ArchiePayne
Copy link
Contributor

@ArchiePayne ArchiePayne commented Jan 11, 2026

Describe your changes

Implement the Normal Rendering mode, displaying arrows on top of the geometry to depict vertex normals.

Added option --normal-glyphs with binding ctrl+n to toggle on/off.

Added auto-test for the functionality.

Issue ticket number and link if any

Issue #4

Checklist for finalizing the PR

  • I have performed a self-review of my code
  • I have added tests for new features and bugfixes
  • I have added documentation for new features
  • If it is a modifying the libf3d API, I have updated bindings
  • If it is a modifying the .github/workflows/versions.json, I have updated docker_timestamp

Continuous integration

Please write a comment to run CI, eg: \ci fast.
See here for more info.

@ArchiePayne
Copy link
Contributor Author

\ci fast

@ArchiePayne ArchiePayne force-pushed the normal_rendering_mode branch from 4de79a4 to ba33574 Compare January 13, 2026 19:37
@Meakk
Copy link
Member

Meakk commented Jan 17, 2026

That looks good. I've run the main CI but I think you will have to add a test to cover the warning. Let's see what the coverage check says.

@ArchiePayne ArchiePayne force-pushed the normal_rendering_mode branch from 8838220 to 7c6f3ab Compare January 25, 2026 17:54
@codecov
Copy link

codecov bot commented Jan 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.71%. Comparing base (3c78479) to head (1d61c7b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2811      +/-   ##
==========================================
- Coverage   96.73%   96.71%   -0.02%     
==========================================
  Files         154      154              
  Lines       13256    13291      +35     
==========================================
+ Hits        12823    12855      +32     
- Misses        433      436       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ngs.GlyphMapper->SetSourceConnection(arrowSource->GetOutputPort());
ngs.GlyphMapper->SetOrientationModeToDirection();
ngs.GlyphMapper->SetOrientationArray(vtkDataSetAttributes::NORMALS);
ngs.GlyphMapper->SetScaleFactor(0.2);
Copy link
Member

Choose a reason for hiding this comment

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

should that depends on the bounds of the data ?

ngs.InputDataHasNormals = points->GetPointData()->GetNormals() != nullptr;

vtkNew<vtkArrowSource> arrowSource;
ngs.GlyphMapper->SetInputData(points);
Copy link
Member

Choose a reason for hiding this comment

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

should you use masking ?

this->ColoringPointSpritesMappersConfigured = true;
}

//// Handle Normal Glyphs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//// Handle Normal Glyphs
// Handle Normal Glyphs


/**
* Set the visibility of the normal glyphs actor.
* It will only be shown if raytracing and volume, and Point Sprites are not enabled
Copy link
Member

Choose a reason for hiding this comment

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

this comment is not super clear

Copy link
Member

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

changes needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants