Skip to content

Introduction of Boolean Spec in lib3mf#461

Merged
vijaiaeroastro merged 8 commits into
developfrom
feature/booleans
May 13, 2026
Merged

Introduction of Boolean Spec in lib3mf#461
vijaiaeroastro merged 8 commits into
developfrom
feature/booleans

Conversation

@vijaiaeroastro

Copy link
Copy Markdown
Collaborator

No description provided.

@codecov

codecov Bot commented Apr 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.67925% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.04%. Comparing base (8632cc9) to head (7a3231f).

Files with missing lines Patch % Lines
Autogenerated/Bindings/Cpp/lib3mf_implicit.hpp 88.67% 12 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #461      +/-   ##
===========================================
+ Coverage    59.66%   60.04%   +0.38%     
===========================================
  Files           64       67       +3     
  Lines        24646    25269     +623     
===========================================
+ Hits         14705    15173     +468     
- Misses        9941    10096     +155     

☔ 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 April 29, 2026 11:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

Comment thread Source/API/lib3mf_model.cpp Outdated
Comment thread Source/Model/Writer/v100/NMR_ModelWriterNode100_Model.cpp
Comment thread Source/Model/Writer/v100/NMR_ModelWriterNode100_Model.cpp
throw CNMRException(NMR_ERROR_INVALIDPARAM);
if (dynamic_cast<CModelComponentsObject *>(pObject) != nullptr)
throw CNMRException(NMR_ERROR_INVALIDOBJECT);
if (dynamic_cast<CModelLevelSetObject *>(pObject) != nullptr)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Per Boolean Operations Extension v1.1.1 §2.1: "The object MUST be an object of type 'model' defining a shape: mesh, booleanshape, or shapes defined in other 3MF extensions."

Level sets (Volumetric 2022/01) are exactly "shapes defined in other 3MF extensions" — the spec only excludes . Both setBaseObject and isValid reject them. This also makes the matching test ApiRejectsLevelSetAsBaseObject (Tests/CPP_Bindings/Source/Boolean.cpp:347-353) actively assert non-conformant behavior.

Suggest: drop the CModelLevelSetObject checks at lines 105 and 275, and invert the test to assert acceptance.

m_pModel->addResource(m_pObject);

if (m_bHasDefaultPropertyIndex || m_bHasDefaultPropertyID)
m_pWarnings->addException(CNMRException(NMR_ERROR_OBJECTLEVELPID_ON_COMPONENTSOBJECT), mrwInvalidOptionalValue);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

pid / pindex on booleanshape only emits a warning (writer doesn't enforce at all)

Spec §Chapter 2: "producers MUST NOT assign pid or pindex attributes to objects that contain booleanshape."

Two issues here:

The error code NMR_ERROR_OBJECTLEVELPID_ON_COMPONENTSOBJECT is semantically wrong — this is a booleanshape, not a components object. Add a _ON_BOOLEANOBJECT variant or a generic name.
Nothing stops a producer path from writing pid on a boolean-bearing object — writeBooleanObject (NMR_ModelWriterNode100_Model.cpp:675) doesn't check. Add a writer-side check that throws if the boolean object carries object-level properties, mirroring how is treated.

// Temporary realization path: flatten referenced meshes with transforms.
// This preserves object traversal/export behavior until true CSG evaluation is added.
for (const auto & operandMesh : operandMeshes)
pWorkingMesh->mergeMesh(operandMesh.get(), fnMATRIX3_identity());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This produces a union of all parts for every operation type. Any consumer that calls mergeToMesh (e.g., an STL writer, slicer pipeline) will silently get incorrect geometry for difference and intersection shapes unless CSGModeEnabled is explicitly set to true.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

When m_bCSGModeEnabled == false, the result is base ∪ operandMeshes regardless of the requested operation. A consumer calling WriteToBuffer("stl") on a Difference boolean object gets a union with no warning. Options:

Emit a model warning when m_eOperation != Union and CSG mode is disabled.
Require CSGModeEnabled = true for non-Union operations (throw otherwise).

Comment thread Source/API/lib3mf_booleanobject.cpp Outdated
throw ELib3MFInterfaceException(LIB3MF_ERROR_INVALIDMESHOBJECT);

auto pResource = booleanObject()->getModel()->findResource(pMeshObject->getPackageResourceID());
pOperandObject = new CMeshObject(pResource);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

// Dispatch through the factory, then assert the spec-mandated mesh-only operand type.
std::unique_ptr<IObject> pObject(CObject::fnCreateObjectFromModelResource(pResource, true));
auto * pMesh = dynamic_cast<IMeshObject *>(pObject.get());
if (!pMesh)
    throw ELib3MFInterfaceException(LIB3MF_ERROR_INVALIDMESHOBJECT);

pObject.release();

@@ -0,0 +1,486 @@
/*++

Copyright (C) 2026 tensorGrad Consulting LLP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure about this. Should it not be Copyright (C) 2026 3MF Consortium + a note that this was contributed by tensorGrad Consulting LLP? You deserve the credit, but we do usally not name the companies that contributed to lib3mf explicitly, they are all contributing as 3mf consortium.

@alexanderoster what is your opinion on this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@3dJan Actually i will remove this. I already discussed this with @gangatp

@vijaiaeroastro

Copy link
Copy Markdown
Collaborator Author

@gangatp @3dJan I have addressed all the comments in PR. Review again please.

@vijaiaeroastro vijaiaeroastro marked this pull request as ready for review May 7, 2026 05:45
@vijaiaeroastro vijaiaeroastro merged commit 82c524d into develop May 13, 2026
44 checks passed
@vijaiaeroastro vijaiaeroastro deleted the feature/booleans branch May 13, 2026 13:55
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