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

Try to abolish Shape #697

Closed
hannobraun opened this issue Jun 18, 2022 · 3 comments · Fixed by #747
Closed

Try to abolish Shape #697

hannobraun opened this issue Jun 18, 2022 · 3 comments · Fixed by #747
Assignees
Labels
topic: core Issues relating to core geometry, operations, algorithms type: development Work to ease development or maintenance, without direct effect on features or bugs

Comments

@hannobraun
Copy link
Owner

hannobraun commented Jun 18, 2022

Shape is the central data structure that fj-kernel uses to store the objects that make up a shape. It was initially introduced for the following reasons:

  • Validation: Storing objects in a central location gives you a point where you can implement validation logic. I'm confident that there are solutions to achieve validation without Shape though (see Decouple validation infrastructure from Shape #696).
  • Object identity: An initial idea was to create objects once, and only refer to the existing object from then on, instead of allowing identical objects to be created. This idea turned out to be not that necessary, given that the validation infrastructure already prevents a lot of errors, and was later largely sacrificed for the sake of convenience (see builder API and its supporting infrastructure).
  • Immutability: Part of the initial idea was to make objects stored in Shape immutable. This was later changed to support use cases of various algorithms (namely transform and sweep).

So as it turns out, Shape didn't end up achieving its core goals. But it does bring with it a lot of complexity. So far, I've considered this just a cost of doing business, but I recently realized that it should be possible to remove Shape completely, which would be a radical simplification of the kernel data structures.

I'm thinking about the following measures:

  • There's already a firm plan in place for validation. See the aforementioned Decouple validation infrastructure from Shape #696.
  • Object identity is simply not important right now, and it can be ignored.
  • Immutability can be achieved in simpler ways, using standard Rust practices. The algorithms that rely on object mutability can probably be updated not to. If I recall correctly, transform only needs that because the complexity brought by Shape made its implementation really complicated, and introducing mutability was a simplification. For sweep, I have various vague ideas that I still need to explore.

My idea for the Shape-less future is to simply keep the object types as they are right now, roughly, with two important differences:

  • Make all their fields private, and only allow access to them via accessor methods that return shared references.
  • Instead of referring to other objects via Handles, just have each objects own a copy of the referred to object.
    • Any potential for inconsistency can be taken care of by the validation infrastructure.
    • The redundancy this introduces is likely not critical right now, as there's a lot of redundancy already between different instances of Shape. I don't think things will get any worse, performance-wise.
    • If the redundancy ever turns into a problem (for example, though too-high memory usage), maybe some simpler form of central data structure can be re-introduced later.
  • Maybe some supporting infrastructure will be required to manipulate objects efficiently and conveniently. That is yet to be determined.

So that's basically it. Many details are yet to be figured out, but this is the rough direction that I'd like to go in.

This issue isn't blocked right now, as all of the areas it covers can be worked on or experimented with immediately, but its completion is blocked on #696.

@hannobraun hannobraun added type: development Work to ease development or maintenance, without direct effect on features or bugs topic: core Issues relating to core geometry, operations, algorithms labels Jun 18, 2022
@hannobraun hannobraun self-assigned this Jun 18, 2022
@hannobraun
Copy link
Owner Author

#696 is finished. I'm now exploring more ways to detach Shape from the rest of the code, so it can be removed, eventually.

@hannobraun
Copy link
Owner Author

I just merged #723, a complete rewrite of the sweep algorithm. This has kept me busy for a few days, and is a huge step towards removing Shape. I can't be sure if any surprises are still lurking, but I dare say, the most difficult hurdles have probably been taken.

@hannobraun
Copy link
Owner Author

This is mostly done. Very little code still uses Shape. Most (or all?) of the code that does, is related to tests that can now be cleaned up.

Really close to wrapping this up, but unfortunately I've run out of time for today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Issues relating to core geometry, operations, algorithms type: development Work to ease development or maintenance, without direct effect on features or bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant