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

Add check in Extrude(), replace replace codecvt_utf8_utf16 #1282

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ifcapps
Copy link
Contributor

@ifcapps ifcapps commented Feb 18, 2025

In Extrude(): check if first point is equal to last point, otherwise the outer loop of the shape is not closed
In string_parsing.cpp, replace codecvt_utf8_utf16 which is deprecated in C++17

ifcapps and others added 7 commits February 18, 2025 17:51
- enable polylines as mesh for IFCGRID, IFCGRIDAXIS
- add bounds checks in TriangulateRevolution
make scaling by _geometryLoader.GetLinearScalingFactor()  optional in GetFlatMesh.
Alternative would be: make IfcGeometryProcessor::AddComposedMeshToFlatMesh public
@beachtom
Copy link
Collaborator

Thanks for this!

The following IFC files in the tests output has changed. Can you check the changes are positive i.e. have improved the rendering and then you need to re run npm run update-regression to update the result cahce

tests/ifcfiles/public/AC20-FZK-Haus.ifc- FAIL
tests/ifcfiles/public/C20-Institute-Var-2.ifc- FAIL
tests/ifcfiles/public/FM_ARC_DigitalHub.ifc- FAIL
tests/ifcfiles/public/ISSUE_005_haus.ifc- FAIL
tests/ifcfiles/public/ISSUE_021_Mini Project.ifc- FAIL
tests/ifcfiles/public/ISSUE_034_HouseZ.ifc- FAIL
tests/ifcfiles/public/ISSUE_044_test_IFCCOMPOSITEPROFILEDEF.ifc- FAIL
tests/ifcfiles/public/ISSUE_053_20181220Holter_Tower_10.ifczip- FAIL
tests/ifcfiles/public/ISSUE_068_ARK_NUS_skolebygg.ifc- FAIL
tests/ifcfiles/public/ISSUE_102_M3D-CON-CD.ifc- FAIL
tests/ifcfiles/public/ISSUE_126_model.ifc- FAIL
tests/ifcfiles/public/ISSUE_129_N1540_17_EXE_MOD_448200_02_09_11SMC_IGC_V17.ifc- FAIL
tests/ifcfiles/public/ISSUE_159_kleine_Wohnung_R22.ifc- FAIL
tests/ifcfiles/public/ISSUE_171_IfcSurfaceCurveSweptAreaSolid.ifc- FAIL
tests/ifcfiles/public/IfcOpenHouse_IFC4.ifc- FAIL
tests/ifcfiles/public/KIT-Simple-Road-Test-Web-IFC4x3_RC2.ifc- PASS
tests/ifcfiles/public/Office_A_20110811.ifc- FAIL
tests/ifcfiles/public/S_Office_Integrated Design Archi.ifc- FAIL
tests/ifcfiles/public/Sample_entities.ifc- PASS
tests/ifcfiles/public/advanced_model.ifc- FAIL
tests/ifcfiles/public/dental_clinic.ifc- FAIL
tests/ifcfiles/public/duplex.ifc- FAIL
tests/ifcfiles/public/example.ifc- FAIL
tests/ifcfiles/public/ifcbridge-model01.ifc- PASS
tests/ifcfiles/public/schependomlaan.ifc- FAIL
tests/ifcfiles/public/tested_sample_project.ifc- FAIL

…TESIANPOINT as geometric item

IFCFACESURFACE, IFCEDGE and IFCCARTESIANPOINT are derived from IfcRepresentationItem, so they could appear as geometric item

In IfcGeometryLoader::GetColor: add check for curveColour because it is optional
@ifcapps
Copy link
Contributor Author

ifcapps commented Feb 28, 2025

During testing the files in tests/ifcfiles/public/ a few issues came up which are addressed in this pr: #1300
The tests now run through, at least locally on my machine. The loaded meshes look good

@ifcapps
Copy link
Contributor Author

ifcapps commented Mar 12, 2025

Hi,
should the test models run without these messages:

Error: [WEB-IFC][error][TriangulateBounds() Expected outer bound first! 109816?

When I load these models even with the unchanged code, there are many such messages.

Maybe the outer bound error could be classified as warning? Since these are quite common.

@beachtom
Copy link
Collaborator

So yes getting that is normal - so as long as the models look visually OK - then it is fine

@ifcapps
Copy link
Contributor Author

ifcapps commented Mar 13, 2025

I am not sure if it is possible to visually check the models converted by the test workflow.
But I always run my local models, and they were ok with these changes.

@beachtom
Copy link
Collaborator

OK fab - that's what I meant. Can you run npm run regression-update and then push those changes. Or give me access to update the PR and I can do it. Does #1300 need to merged first - or at the same time? Because that still has errors

@ifcapps
Copy link
Contributor Author

ifcapps commented Mar 13, 2025

I did run "npm run regression-update", then pushed the changes (results.json), but it still looks the same.
Can you help me with that?
You have now access to the repo https://github.com/creoox/engine_web-ifc
Thanks a lot!

#1300 can be merged at the same time

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.

2 participants