Skip to content

Conversation

@3dJan
Copy link
Contributor

@3dJan 3dJan commented Sep 14, 2025

  • Writer now detects ball usage up‑front and (when needed) declares xmlns:b2 and emits b2:balls, b2:ball, and b2:ballref instead of the previous (non‑spec) <b:ball*>.
  • Added new constants for the balls namespace/prefix and integrated them into required extension handling.
  • Reader extended to:
  • Accept the balls namespace for / elements.
  • Parse namespaced b2:ballmode and b2:ballradius attributes.
  • Treat the balls namespace as a supported required extension (removes prior warning).
  • Remain backward compatible with legacy files using <b:balls> / <b:ball>.
  • New test 3MF asset plus test validating: no warnings, ball mode = All, default radius = 3.0, total implicit balls = vertex count (8).
  • Added a namespace compliance test ensuring serialization uses b2 and never falls back to legacy b:ball forms.

@3dJan 3dJan changed the base branch from master to develop September 14, 2025 18:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements proper namespace support for beam lattice balls in the 3MF specification by introducing a dedicated balls namespace (b2:) to replace the non-compliant legacy b:ball* elements. The changes ensure spec compliance while maintaining backward compatibility.

Key changes:

  • Adds detection logic to determine when balls extension is needed and declares appropriate namespace
  • Updates writer to emit proper b2:balls, b2:ball, and b2:ballref elements instead of legacy forms
  • Extends reader to accept both new balls namespace and legacy b:ball* for backward compatibility

Reviewed Changes

Copilot reviewed 19 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Source/Model/Writer/v100/NMR_ModelWriterNode100_Model.cpp Adds ball usage detection and namespace declaration logic
Source/Model/Writer/v100/NMR_ModelWriterNode100_Mesh.cpp Updates to use balls namespace prefix for ball elements
Source/Model/Reader/BeamLattice1702/NMR_ModelReaderNode_BeamLattice1702_BeamLattice.cpp Adds support for namespaced ball attributes
Include/Model/Classes/NMR_ModelConstants.h Defines new balls namespace constants
Tests/CPP_Bindings/Source/BeamLattice.cpp Adds test for balls extension compliance

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@3dJan
Copy link
Contributor Author

3dJan commented Sep 14, 2025

just noticed that in CI build https://github.com/3dJan/lib3mf/actions/runs/17714380031/job/50337632342 the
integration-test-last-commit-and-last-release failed

3dJan and others added 4 commits September 18, 2025 20:38
fixing initialization order

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…t.cpp


fixing initialization order

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ilures and MUSTFAIL passes for latest release and commit
@3dJan
Copy link
Contributor Author

3dJan commented Sep 19, 2025

There seems to be indeed a regression, two negative tests pass now:
=== Regressions (commit vs release) - MUSTFAIL now passing:
/home/runner/work/lib3mf/lib3mf/test_suites/suite7_beam/negative_test_cases/N_BXX_2506_01.3mf
/home/runner/work/lib3mf/lib3mf/test_suites/suite7_beam/negative_test_cases/N_BXX_2506_07.3mf

Btw. I now also extended the integration tests to list the test cases with regressions.

@codecov
Copy link

codecov bot commented Sep 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.08%. Comparing base (fce4988) to head (400edfe).
⚠️ Report is 19 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #433   +/-   ##
========================================
  Coverage    59.08%   59.08%           
========================================
  Files           64       64           
  Lines        23589    23589           
========================================
  Hits         13937    13937           
  Misses        9652     9652           

☔ 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.

@gangatp gangatp requested a review from Copilot October 8, 2025 11:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Collaborator

@gangatp gangatp left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Collaborator

@vijaiaeroastro vijaiaeroastro left a comment

Choose a reason for hiding this comment

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

Looks good, Jan. Thanks for the changes in integration tests too.

@3dJan 3dJan merged commit b9cdaed into develop Oct 8, 2025
42 checks passed
@3dJan 3dJan deleted the 3dJan/Beamlattice_namespace_fix branch October 8, 2025 13:08
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.

4 participants