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

[Test Case]: Alignment Infrastructure Curves #26

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from

Conversation

larswik
Copy link
Collaborator

@larswik larswik commented Jan 10, 2022

Summary

Outline of the content and tasks addressed by this pull request, dont forget to link the relevent issue(s)

Purpose

Please mark the relevent tasks that this pull request creates or updates:

Exchanges

  • Creation of new exchange
  • Documentation of exchange

Test Cases

  • Creation of new test case
  • Test case summary
  • Itemised roots & test case imports
  • Usages, constraints & logic
  • Validation criteria & Results
  • Dataset Content & Documentation

General

  • CI updates
  • Repository documentation
  • Bug fixes

@larswik larswik self-assigned this Jan 10, 2022
@larswik larswik added documentation Improvements or additions to documentation test case issues & pull requests relating to test cases labels Jan 10, 2022
Copy link
Member

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

Some suggestions, a lot is still to be defined.

E1a-ARSE/ALIN/Dataset/README.md Outdated Show resolved Hide resolved
E1a-ARSE/ALIN/README.md Outdated Show resolved Hide resolved
E1a-ARSE/ALIN/README.md Outdated Show resolved Hide resolved
Comment on lines 98 to 101
| TI Code | Test Instruction Title | Comments |
| ---------------------------------------- | ----------------------- | ------------------------------------------------------------ |
| [IFC4.3AbRV_E0_SSRD](../../E0-SSRD/SSRD) | Spatial Structures Road | Spatial structure for road incuding the dependencies (E0_SSSI, E0_MSTP) |

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a georeferencing TI?

Copy link
Collaborator Author

@larswik larswik Jan 10, 2022

Choose a reason for hiding this comment

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

Thanks for commenting @pjanck! I have assumed that georeferencing is part of E0_MSTP which is mentioned in the table (and if you take a look into the TI dependency diagram from @AlexBrad1eyCT) on Miro https://miro.com/app/board/o9J_lkyDq70=/.
As you say, there's a lot still to be defined. My main issues are:

  • I have assumed straight line, circular arc, clothoid for horizontal and straight line, parabolic or circular arc for vertical. Is that all for a road alignment or are there other more obscure cases?
  • Is it enough to test straight, left turn, right turn in horizontal and straight up/down (connected with arcs) for vertical?
  • How to produce the actual test case? I looked at e.g. the alignment from MCON-2 but that does not contain clothoids. It would be nice to have a set case that uses all necessary segment types in all typical ways (left/right, up/down, transitions, ...).

Copy link

Choose a reason for hiding this comment

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

For a road alignment, even parabolic arc is rare enough to raise the question: is it going to be mandatory for certification? Looking at validation criteria "For this test case to be considered passed all capabilities listed in this section shall be verified, with no exception" and "At least 1 instance of each entity listed in Itemised Roots is present in the file", I'd be very careful with what is included in the case (should be testing IFC export/import, not software capability in general). If it's not against any chosen certification strategy (?) might be reasonable to allow also "passed with exceptions (documented)", but if the validation criteria for a case really must be that draconian, I'd suggest making first a basic road alignment case with linear, circular arc and clothoid for horizontal; linear and circular arc for vertical.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

During my "peak days", as a developer of road design software (this was a long time ago), the only vertical "curved curve" used was parabolic arc. Unfortunately, I didn't find anything regarding this in the RA. Might have missed it though. I wanted to mention both circular and parabolic until more info available

Copy link

Choose a reason for hiding this comment

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

@larswik I'm not saying that parabolic arc wouldn't be valid vertical curve on roads and shouldn't be included in some test case. I'm just asking does it have to be supported as a mandatory requirement? If a software doesn't do parabolic arcs since their users do not use them they should still be able to get certified - I hope.

Copy link
Collaborator Author

@larswik larswik Jan 11, 2022

Choose a reason for hiding this comment

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

@JuhaHy good point of course! But it goes both ways right? I'm not sure how to organize these variations, but let's see.

Copy link
Member

Choose a reason for hiding this comment

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

Meta: I'd move this discussion to a separate issue for easier traceability. Searching for these comments in the future will be a pain, as well as a separate issue enables others to chime in as well. @larswik I'll copy the comments if that is ok with you.

georeferencing is part of E0_MSTP which is mentioned

That answers my question and resolves this conversation.


The valid geometry segment types for horizontal are the following:

- Arc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would we just add parabolic arc here since the rest would be covered in E1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that we could do it like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping @pjanck

Copy link
Member

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

So far so good!

Copy link
Member

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

So far so good!

Comment on lines 31 to 33
- Line => Circular arc (cw) => Circular arc (ccw) => Line

- Line => Circular arc (ccw) => Circular arc (cw) => Line
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps better to use "crest" and "sag"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed now.

Comment on lines 37 to 38
- IfcSite
- IfcRoad
Copy link
Member

Choose a reason for hiding this comment

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

I'd list this in Spatial structure, separately from Model setup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed now.

Copy link
Member

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

Reviewing while work is being done <3 I'll look at this later.

Comment on lines 36 to 38
| A1 | IfcAlignment | IfcLocalPlacement | None |
| AH1 | IfcAlignmentHorizontal | IfcLocalPlacement | None |
| AV1 | IfcAlignmentVertical | IfcLocalPlacement | None |
Copy link
Member

Choose a reason for hiding this comment

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

I'd not expect ObjectPlacement if there is no Representation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed now.

Copy link
Member

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

I'm not done yet, but it's something :)

E1a-ARSE/ALIN01/Dataset/README.md Outdated Show resolved Hide resolved
E1a-ARSE/ALIN01/Dataset/VerticalAlignmentParameters.csv Outdated Show resolved Hide resolved
E1a-ARSE/ALIN01/README.md Outdated Show resolved Hide resolved
Copy link
Member

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

Seems that my comments have been addressed.

The PR is still a draft. I suppose, more is going to be added?

@larswik larswik removed the request for review from AlexBrad1eyCT March 11, 2022 16:16
@larswik larswik marked this pull request as ready for review March 11, 2022 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation test case issues & pull requests relating to test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants