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

[WIP] Implementing new unit tests for IFC4X3_ RC4 #478

Open
wants to merge 11 commits into
base: development
Choose a base branch
from

Conversation

Elvira2227
Copy link
Collaborator

  • Some unit tests have been added as an example for the version IFC4X3_ RC4

  • All tests are missing a IFCHasAnEssentialEntity test, please add

  • No screenshots were taken. For example, only the one main screenshot is defined for a clothoid, an error is generated for the rest.

NOTE: this pull request was created to replace the old #477, because the old PR was showing changes from other reguest, which is an error

@Elvira2227 Elvira2227 self-assigned this Oct 14, 2021
@Elvira2227 Elvira2227 added example-files Issue related to failing reading an example file IFC Content related to Industry Foundation Classes (IFC) functionalities labels Oct 14, 2021
Copy link
Collaborator

@jschlenger jschlenger left a comment

Choose a reason for hiding this comment

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

Looks fine to me!

Copy link
Contributor

@pjanck pjanck left a comment

Choose a reason for hiding this comment

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

General comment: test for IfcSpiral and IfcCurveSegment (or how it's called) in the HasEssentialIfcEntity tests.

Comment on lines +18 to +38
#pragma once
#ifndef F7302D7B_948F_445E_8DB9_C722295B8E2E
#define F7302D7B_948F_445E_8DB9_C722295B8E2E

#if !defined OIP_NAMESPACE_UNITTESTS_SCHEMA_IFC4X3_RC4_BEGIN
#define OIP_NAMESPACE_UNITTESTS_SCHEMA_IFC4X3_RC4_BEGIN \
namespace OpenInfraPlatform {\
namespace UnitTests {\
namespace Schema {\
namespace IFC4X3_RC4 {
#endif

#if !defined OIP_NAMESPACE_UNITTESTS_SCHEMA_IFC4X3_RC4_END
#define OIP_NAMESPACE_UNITTESTS_SCHEMA_IFC4X3_RC4_END \
}\
}\
}\
}

#endif
#endif /* F7302D7B_948F_445E_8DB9_C722295B8E2E */
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these defines used anywhere?

Copy link
Collaborator

@jschlenger jschlenger left a comment

Choose a reason for hiding this comment

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

Generally take a closer look on which IfcEntities are really essential to make the UT work.

@Elvira2227
Copy link
Collaborator Author

Elvira2227 commented Jan 27, 2022

In the last commit new unit tests were added for clothoid and line. The tests also contain images.
There are some problems with previos geometries:

  1. Incircular-arc and helmert-curve there are visible geometry on the UI, but some tests (VertexViews and PlaneSurfaceViews) are failed.
  2. In cosine-curve and sine there are no visible geometry in the UI. And also some tests are failed
  3. cubic, viennese-bend produce errors by testing geometry in the UI

And some screen shots:

image

image

@Elvira2227
Copy link
Collaborator Author

Elvira2227 commented Jan 27, 2022

And there is a question about how does the AllEntitiesAreRead work? or how must it work correctly?
Usually we should count the numbers of entities, but in the last test it counts only the number of the type unsigned __int64 (and what does it mean exactly?). For example, in clothoid there are 80 entities and while launching local debugger the test AllEntitiesAreRead fails (see a screen shot).

image

Copy link
Collaborator

@jschlenger jschlenger left a comment

Choose a reason for hiding this comment

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

As discussed live:

  • only top view (all the other views can be deleted because there are only 2D lines)
  • new issue for IfcSine (since we are missing the corresponding GetDirection and GetPoint function)
  • Files that produce error messages have missing references in the IFC file

@Elvira2227
Copy link
Collaborator Author

In the last commit 71f684b screenshots were updated. New screenshots contain only main and top view (Unnecessary Test was deleted).

  • For bloss, helmeert, sine, cosine we need to add corresponding functions to impement them (See [REQUEST]Implementing corresponding functions for IfcSpline #524 )
  • Cubic and viennese-bend curves were not impemented and the problem with IFC file was not found. (Like in helmert or in clothoid files)
  • Clothoid and Lines completed with producing unit tests

@Elvira2227 Elvira2227 requested a review from jschlenger February 8, 2022 16:43
Copy link
Collaborator

@jschlenger jschlenger left a comment

Choose a reason for hiding this comment

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

The name of circular-arc_100.0_300_1000_1_Meter_top123.png should be changed slightly since we just put that name for a quick test (or maybe the file isn't needed anymore then you should delete it).

Although the unit tests for cosine curve, sine curve, vienesse bend, and cubic curve do not work yet I would suggest to adapt the UTs to compare only the top view.

@Elvira2227 Elvira2227 requested a review from jschlenger February 9, 2022 14:05
Copy link
Collaborator

@jschlenger jschlenger left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Except for the four UTs that are still missing a couple of functions to work properly all of the UTs are passing?

Copy link
Contributor

@pjanck pjanck left a comment

Choose a reason for hiding this comment

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

Two small comments.

buw::storeImage(testPath("circular-arc_100.0_300_1000_1_Meter_right.png").string(), image_right);
buw::storeImage(testPath("circular-arc_100.0_300_1000_1_Meter_back.png").string(), image_back);
*/
//buw::storeImage(testPath("circular-arc_100.0_300_1000_1_Meter_top123.png").string(), image_top);
Copy link
Contributor

Choose a reason for hiding this comment

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

A ...top123 image is committed as well. Is there a specific reason? Remove otherwise.

#36 = IFCCURVESEGMENT(.DISCONTINUOUS., #42, IFCPARAMETERVALUE(0.), IFCPARAMETERVALUE(100.), #45);
#36 = IFCCURVESEGMENT(.DISCONTINUOUS., #42, IFCPARAMETERVALUE(0.), IFCNONNEGATIVELENGTHMEASURE(100.), #45);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that we had to change this?

Copy link
Collaborator Author

@Elvira2227 Elvira2227 Feb 18, 2022

Choose a reason for hiding this comment

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

I thought there was a mistake there, because in other ifc's always come IFCPARAMETERVALU and than IFCNONNEGATIVELENGTHMEASURE. Is it correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it doesn't matter, so you can leave it as is or change it: I only have a preference to not change the original file if not necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example-files Issue related to failing reading an example file IFC Content related to Industry Foundation Classes (IFC) functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants