-
-
Couldn't load subscription status.
- Fork 837
Import alignment from csv #6234
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - thanks!!! The humble alignment.py is ready to spread its wings so to speak and become part of the api.
I made suggestions on current year in the copyright headers but not certain if those actually should be updated.
General suggestions for follow-up:
- lint with black
- docstrings for everything in the api (including examples)
- unit tests for the alignment api using the FHWA data as example
Some of these may be good candidates for tasks to put on the infra authoring project backlog.
src/ifcopenshell-python/ifcopenshell/api/alignment/alignment.py
Outdated
Show resolved
Hide resolved
|
Regarding copyright changes I think sooner or later a script that parses git logs per file and updates the copyright dates is probably something we should do project-wide. |
|
Thanks for the feedback. Looks like I’ve got some work to do before this is ready for prime time. |
|
@civilx64 I've been doing a lot of refactoring of the API - it is way more granular now. |
|
You may need to add your module to the list of modules in
`bim/__init__.py`
|
|
@RickBrice apologies; it looks like my docstring exmples are using a different (possibly obsolete) syntax. The previous when compared to other parts of the docs See this example for correct syntax in the docstring and the corresponding reference documentation. If the parameters and return types have python type hints - like this Lastly you should be able to use the doctest module to test any example code in docstrings. Full disclosure - I have never utilized this myself. LMK if you need some help with the busy work I created of reformatting |
|
I'm done a huge amount of refactoring and response to your comments. |
|
@Moult I have updated the basic documentation for the Import > Alignment comment. I have two questions for you regarding documentation
|
a2b3492 to
891a9c4
Compare
|
Looks good to me. Really amazing to see the first contours of infra authoring. Did you have a look at the failing tests: I'm a fan of pytest.approx: The |
|
@aothms I updated the unit tests to use pytest.approx() as suggested. I am having difficulty reconciling the test errors. I don’t think I caused any of the mathutil errors. For geometry settings, I added a new setting, compute-curvature, but don’t know how to update the test for the new setting. There is another setting error related to cgal, and don’t think it is related to my changes. Please advise |
Anywhere in here is fine: https://github.com/IfcOpenShell/IfcOpenShell/tree/v0.8.0/src/bonsai/docs/guides - we can always reorganise it later but content is king :)
I don't have a good answer for this, because image editing takes time and a pixel-perfect attitude to give off a good impression for official docs, but it's also something that we don't have much time for. Perhaps gradiented boxes is too hard. Maybe just file a bug and let someone else help? |
|
@aothms i fixed all the unit tests related to my changes. The remaining failures seem to be related to mathutils. Is there something more I need to to do? @civilx64 any additional comments? If not, I think this PR is ready to merge. We will have a simplistic method of defining an alignment and a starter API. |
…nsai code into a new alignment module)
e28512d to
97862a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a final look over everything - very impressive! Just one typo and an FYI.
src/ifcopenshell-python/ifcopenshell/api/alignment/add_stationing_to_alignment.py
Show resolved
Hide resolved
Co-authored-by: Scott Lecher <civilx64@gmail.com>
|
@Moult I think I messed up with this PR. I merged this branch into the v0.8.0 branch because I thought everything was good to go. I have my Bonsai setup so it uses the latest build from the raw.githubusercontent.com repository. When I updated and tried to use the new alignment from CSV feature it crashes (traceback is below). The alignment feature depends on changes made in IfcOpenShell. Specifically, there is a new geometry setting to compute curvature on an alignment curve. Sorry I messed this up. How can the problem be resolved? |
|
@RickBrice Did you recompile the C++ extension wrapper as well? If so is it linked to the blender python environment? My hunch is that you didn't actually break anything. I don't think the daily releases include new compilations of the C++ wrapper. |
|
I can compile the C++ and copy the wrapper pyd and py files into the bonsai site-package\ifcopenshell folder and everything works. My concern is that if someone updates bonsai with the latest build and tries to use the alignment import feature, it won't work. Some how ifcos needs to be recompiled and packaged with bonsai. |
It will be, just takes a few days to a week for the wrapper to catch up. CI/CD for the C++ gets kicked off manually, I believe. @aothms do I have that right? |
|
Yeah occasionally we have this as part of our imperfect distribution process. You can code more defensively with try-catch for this transition window, but that's also ugly later on. Most people in the community are used to this... I've started a new build. |
|
Thanks. I was afraid I made a major blunder on my first step into the Bonsai side of things. |




This PR is an attempt to provide a quick and dirty mechanism to define alignment geometry in Bonsai. The idea is to define an alignment in a CSV file and import it. This is a temporary feature and will probably go away someday once I get a better handle on how to develop for Bonsai.
There is a new Alignment (.csv) command on the Import menu.
The CSV file format is:
X1,Y1,R1,X2,Y2,R2 .... Xn-1,Yn-1,Rn-1,Xn,Yn
D1,Z1,L1,D2,Z2,L2 .... Dn-1,Zn-1,Ln-1,Dn,Zn
D1,Z1,L1,D2,Z2,L2 .... Dn-1,Zn-1,Ln-1,Dn,Zn
The first row defines the horizontal alignment by the PI method. The circular curve radii R1 and Rn-1 are "placeholders" and are not used. A value of 0.0 is recommended.
The second and subsequent rows define vertical alignments by the PI method. D is distance along, Z is elevation, and L is the horizontal length of a parabolic vertical curve. Similar to the horizontal circular curves, L1 and Ln-1 are "placeholders" and are not used. A value of 0.0 is recommended.
Example:
Create a new project using the New Project Wizard. Use the IFC4X3 Schema
Select File > Import > Alignment (.csv)
Imported an alignment with two vertical profiles