Ultimaker / CuraEngine

Powerful, fast and robust engine for converting 3D models into g-code instructions for 3D printers. It is part of the larger open source project Cura.
https://ultimaker.com/en/products/cura-software
GNU Affero General Public License v3.0
1.66k stars 871 forks source link

CURA-9830 consolidate polygon classes #2065

Closed wawanbreton closed 1 month ago

wawanbreton commented 2 months ago

This PR is a refactoring of the base geometry classes that are used all around CuraEngine. The main changes are:

Initially based on CURA-9830 But also implements CURA-9830 and CURA-9832

wawanbreton commented 2 months ago

Please be very critical when reviewing this, especially for the naming of the classes/methods/... This new architecture should be very easily understandable, so anything that doesn't intuitively make sense is to be modified.

wawanbreton commented 2 months ago

@rburema some answers to your comments:

In fact, I think the current implementation does a lot more than is asked of in the tickets. Several (originally) out-of-scope tickets also got implemented, it seems (see CURA-9831 for example). It may be better if we discuss these things a bit better beforehand. (Though maybe that happened?)

That is true. I mentioned some tickets in the description, that are also covered by this PR. But maybe there are more. This architecture is something I have discussed a lot with Jelle, and also when making proposals on the #cura-developers channel. I agree that this is a big change and that a bit more agreement from the rest of the team would have been healthy, but at some point I had to move on and I was already spending too much time on it. I implemented and tested a few different architectures, and finally took decisions that are strongly matured and based on experience, so I am pretty sure this one is mostly good. Not perfect, but it never is. Future will tell us :smiley:

  • Some of the classes feel like a wrapper around vector

That is indeed intentional. The first implementation I did used inheritance from vector and that didn't feel right. The golden rule of C++ design patterns is to prefer composition over inheritance as mush as possible. In this case it really makes sense. Moreover, it will really make things easier if we someday decide to e.g. switch from Clipper to Wagyu.

  • Speaking of vectors and iterators and such; we should check if all of this is compatible with ranges

Some parts of the code do use ranges, so it seems to works as expected. What is sad is that you can't actually use iteration over segments with ranges, because the "default" iterators are for points. I don't know what would be the proper way to use ranges over segments, but maybe there is one.

  • Lastly, I want to emphasise that we should get rid of the dynamic casts at least, if at all feasible

I did remove some of them in the process, and if I'm correct there must be very few left. But why would you push for that anyway ? That's somethings I used a lot in the past, especially with QObject inheritance, and it has never failed me anyhow. That is actually a very powerful way of getting information about an object instance.