Proposal on how to pass clipping planes info

[cvillagrasa]

Hi @Moult , this is linked to question 4 from here, and some little fights I'm having with the current API, which make me interested in improving it. Let's see if you like the angle of this PR, or maybe you'd go in another direction.

It consists of a very simple idea to begin with: to just pass a location and a normal vector in order to generate a clipping plane, instead of a whole matrix in which X and Y vectors are going to be ignored.

However, I also feel like passing a simple dict, without any definition and/or validation is what was making things difficult in the first place. So in order to address that, I defined a TypedDict during an initial attempt. Nonetheless, without NotRequired and Unpack from Python 3.11, it's quite unusable. Furthermore, some factory method can be added to validate the dict, but that kind of defeats the purpose of its simplicity to begin with. So to me, this was screaming dataclass. Consequently, I defined a ClippingInfo dataclass and placed it under util.representation.

Another thing, I really miss are type hints. They really help to quickly understand the code, and with which args should the API method be called. In addition, the same issue described above happens with the Usecase classes, specially their arguments. I've tried to think of why the settings dict was being used, but couldn't figure it out. So I tried a dataclass instead (for the geometry.add_wall_representation case), expecting maybe to find the reason, but it just works. Curious to learn the underlying reason if I've missed it.

So with the dataclass also in the Usecase, default values can be super easily defined. I've also added some manual validation methods, which don't cover everything, but do cover 100% of their scope. Meaning that they issue a warning if any data has an unexpected format and is going to be ignored. I feel this is super important in general, since as it is now, invalid arguments are ignored silently, making the user have a harder time chasing issues. At the same time, it's not a Pydantic BaseModel validating everything and adding significant overhead, the overhead of a slotted dataclass is negligible. And as it is in this proposal, the dev chooses what to validate (namely, correct keys of dicts or correct classes of entity_instances).

In case this is acceptable, it can be extended to add_profile_representation, add_slab_representation and their respective calls from BlenderBIM, using a dict with "location" and "normal", instead of with "matrix". And of course, the conversion of the Usecase could be extended to the whole API. Is there any interference with Sphinx, maybe?

When you have availability, let me know what you think of this, and if there's any drawback concerning dataclasses or type hints I haven't seen. We can screenshare if needed.

[cvillagrasa]

BTW @aothms another superminor topic, but I accidentally found it when messing around for this PR.

In util.placement.get_mappeditem_transformation, a non-existant get_cartesiantransformationoperator is called, whereas the existing function ends in 3d. In any case, my PyCharm linting tells me the function is not being used within the codebase. Coming from 8f8862d

[aothms]

Hm, that's indeed strange, we have something like that here https://github.com/IfcOpenShell/IfcOpenShell/blame/v0.7.0/src/blenderbim/blenderbim/bim/import_ifc.py#L1875-L1879 but it's based on blender's mathutils. I also remember discussing with @Moult that that function isn't entirely correctly implemented, because it can't do mirroring. There is the Axis3 attribute, which is ignored. I really don't remember what happened with this discussion though and why the result is a missing reference in python :(

[aothms]

to just pass a location and a normal vector instead of a whole matrix in which X and Y vectors are going to be ignored

Even less ignored data would be a 4-component plane equation $Ax + By + Cz + D = 0$. If you take the point $P_{xyz}$ and normal vector $N_{xyz}$ from your example $D = - P @ N$ with $@$ the dot product, as it is called in numpy / mathutils. But point plus normal is often easier to deal with unless you're really trying to solve linear equations.

[cvillagrasa]

Awesome! but yeah, I wouldn't save those 2 floats from $\{A, B, C, D\}$ with respect to $\{(p_x, p_y, p_z), (n_x, n_y, n_z)\}$ if we want a friendly API 😝. Also, the 6 components are stored in IFC at the end of the day as Location and Axis of the IfcAxis2Placement3D.

[Moult]

Hey this is a great change, just been delaying merging it since I haven't had the time to check for usages in BBIM and would like to ensure that this doesn't break things in BBIM

[cvillagrasa]

Sure! No problem. Nice that you don't find anything very off at first glance.

I just added a strict validation flag similar to transactions, as discussed here.

[Moult]

Hey just an update this PR looks at a few things. Some I agree with some I don't which is why I'm not just pressing "merge". Here's a breakdown:

1. The clipping plane stuff. Totally agree, I've asked @Andrej730 to help test if this breaks anything on the BBIM side. Once he does this testing, he'll merge in your suggestions with the clipping plane.
2. The util.placement.get_mappeditem_transformation. This has already been fixed in the last release and it also helped fix a bunch of stuff that @aothms spotted in the import_ifc.py. So it's now resolved.
3. Type hinting and dataclass. This overlaps with Patch API calls to ifcopenshell.api #3830. I really don't like magic runtime monkey patching, but I 100% agree with the need for explicit IDE code completion, signature fetching, docstring fetching, type hinting, etc. I workshopped this today a bit with @Andrej730 in a call to come up with a solution and so expect a PR prototype soon from @Andrej730 to get feedback.
4. The strict validation flag. Am I missing something but right now it's just a getter/setter with no implementation? If so I'd like to hold-off until there is actually a more concrete implementation to understand exactly what happens when you enable strict mode.

[cvillagrasa]

Thanks for the follow up, Dion.

1. Great!
2. Great!
3. I think we mainly agree on wanting type hinting benefits, but not seeing runtime patching as a long term solution, let me continue this one on Patch API calls to ifcopenshell.api #3830
4. Enabling a strict mode was your suggestion, so if I misunderstood it just hold it off, sure. As it stands on this PR, it's implemented on the Usecase side. I don't think that how a strict mode is set is super important, as long as there is a way to get the benefits of IDE code completion.

As a side note, I'm making these proposals because I can get immediate benefit from them when using the library, so it just makes sense for me. What's for sure is that this is not intended to put any pressure on the current super dev team (i.e. @Andrej730) on rebuilding my PRs. Let me quote this from the first message two months ago:

We can screenshare if needed.

Maybe I could have been on that call also =)

[Moult]

As it stands on this PR, it's implemented on the Usecase side.

Whoops I missed those two lines. This makes a lot more sense now.

Maybe I could have been on that call also =)

Yes, I'm sorry, the call was actually meant to discuss different topics but we drifted off-topic and ended up chatting about this. We'll stick to text Github from now on.

[Andrej730]

Merged the clippings planes parts to the main branch. Tried not to modify usecases themselves much, the only changes were to make them use ClippingInfo.

[cvillagrasa]

Great @Andrej730 !

Do we want to support Python 3.9 and lower on ifcopenshell-python? I used PEP 604 notation for annotations because it feels cleaner and thought it was sensible to require Python 3.10 in a rapidly evolving library as of today, but maybe Dion and Thomas prefer to type Union[ClippingInfo, None] instead of ClippingInfo | None.

[Andrej730]

Do we want to support Python 3.9 and lower on ifcopenshell-python?

Sure, let's fix it.