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

Create separate presentation layer #2117

Closed
hannobraun opened this issue Nov 29, 2023 · 8 comments · Fixed by #2234
Closed

Create separate presentation layer #2117

hannobraun opened this issue Nov 29, 2023 · 8 comments · Fixed by #2234
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 Nov 29, 2023

Presentation of shapes, how they look, is currently pretty basic in Fornjot: Regions have an optional color. If none is set, it's red.

I think that's perfectly fine for now. While the modeling features are still so primitive, there's really no need for anything else (actually, being able to override the default color in the first place is probably more than we need right now 😄).

However, it's obvious that at some point, we will need something better. Gradients, textures, full material definitions, whatever. But implementing these better methods by adding more attributes to the object graph, does not seem desirable. It would clutter up the object graph with more responsibilities, when I would rather do the opposite. Long-term, we probably don't even want this presentation data to be part of fj-core. Maybe we'll declare it out of scope for the whole project.

Whatever the long-term situation is going to be, I think a sensible step right now would be to move color definitions out of the object graph, into a separate presentation layer. That would simplify the object graph, focusing its scope. And not only would such a change make sense in itself, it would also be a nice opportunity to test-drive the concept of separate layers, serving as preparation for #2116.

@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 Nov 29, 2023
@hannobraun hannobraun self-assigned this Dec 15, 2023
@hannobraun
Copy link
Owner Author

I've finally had a chance to start looking into this seriously, and I've immediately hit a problem. Not just with this issue, but the whole concept of layers.

So, objects are immutable. Updating an object means creating a new object. If layers function by assigning attributes (appearance, geometry, ...) to objects, and updating an object creates a new object, then a naive implementation of layers will not assign any attributes to the new version of an object. This sounds very confusing. To stay within the scope of this issue, if you change anything about an object, you wouldn't expect it to lose its color. If you split a face, you wouldn't expect the colors of the new faces to be reset to default.

This means that we need some mechanism to update the layers along with objects, but it's not clear to me how to do that. It certainly won't do to do it manually, everywhere an object is somehow updated. That would be easy to forget, and thus way too error-prone.

There probably needs to be a new low-level operation (similarly low-level as the insert operation) that is used everywhere to create new versions of objects. derive might be a suitable name, but I'm not sure. This operation could then take care of updating any layers as appropriate. Or maybe, if layers shouldn't be hard-coded, that operation would create some kind of data stream that layers could then process.

It's also not clear to me whether the same layered attributes should apply to all versions of an object. If I update an object, does the same color apply to both its old and new versions (meaning, if the color is changed, it changes for all versions of the object)? Or does updating the object create a copy of the layered data?

Not sure how to proceed from here. This requires some thinking.

@hannobraun
Copy link
Owner Author

I did some thinking, and here's what I've come up with:

  • I've decided to keep it simple for now. Just implement straight-forward solutions that serve this issue specifically, use those to learn more about the problem space.
  • There needs to be some low-level operation for deriving an object from another, and all object updates need to go through that. I'm concerned about robustness, as I can't think of a way to enforce all code to go through that operation. But for now, having that operation will do. Maybe a solution to the problem will emerge later.
  • Deriving an object from another copies the data in the presentation layer, meaning the old and new object can be re-colored separately. I'm not sure that's the right call (eventually, we might want this to work like a graph of modifications, where changing an earlier node applies to all following nodes), but it will do for now, and should lead to new insights.
  • Copying the data in the presentation layer can be hardcoded into the new derive operation. This won't be the final solution, but it will do for now.

@hannobraun
Copy link
Owner Author

I've been making some progress, submitting a bunch of pull requests, but so far only with preparatory work. Now that I'm getting deeper into it, I've hit another hurdle: The derive operation, as I imagined it, can't work.

As I explained above, the derive operation would (in its initial implementation) update the color in the presentation layer, where necessary. The presentation layer would map Handle<Region> to their respective color, if available. That means, the derive operation needs a Handle<T> (e.g. Handle<Region>), as opposed to just a T (e.g. Region).

And this can't work. Lots of places where the derive operation would be required handle Regions that they don't insert yet. Mostly, because the code is deep within some operation, and operations generally don't insert stuff unless necessary. The reason for that is that insertion triggers validation, and any single operation might leave an object in an invalid state (because it might be just one step in a chain of operations that creates a valid object in the end).

I'm not sure what to do about that. One way would be to return some kind of DerivesFrom<T> struct that wraps the un-inserted object and also references the handle to the original object. It could maybe Deref to the wrapped object, for full convenience, but have a custom Insert implementation that updates the presentation layer (and later other layers, as necessary).

I'm not sure yet, if this is a good idea. I think I will think some more on this before experimenting with a solution.

@hannobraun
Copy link
Owner Author

I've done more thinking, and I decided to go with the DerivesFrom<T> idea that I presented in my previous comment. It best fits into the current model and should allow me to move forward, thereby hopefully clarifying the problem space for me.

I suspect that what's happening here is that I'm starting to hit the limits of the current way of doing things. I wrote in a previous message, that a "modification graph" might be a better way to do this (and @HackerFoo has given me advice along those lines in the Matrix room; thank you!), but so far I don't have a good grasp on what that would look like within the context of Fornjot.

I'll keep thinking, but in the meantime, I think it's best to make progress. This will not only enable new features, but also help me learn more about the problems with the current approach. My experience has been that this approach can lead to a big mess in the short-term, but that this mess then leads to learning and better approaches longer-term.

@hannobraun
Copy link
Owner Author

Progress here has been sluggish, as I've been running into small problems at every turn. And now I've been running into a bigger problem that I'm not sure how to handle. Problem is, the derive operation I've been building doesn't work as designed. It can only be used where you have a Handle, and there simply isn't a Handle available everywhere it needs to be used.

I've come to the conclusion that I need to take a step back and reconsider how the larger architecture around operations and layers can be restructured to fit these new requirements. I have some ideas, but those will require some clarification and experimentation. Until then, I'm putting further work here on hold.

@hannobraun
Copy link
Owner Author

I'm happy to report that I am back on track here!

I've prototyped a few different solutions to the problems with the derive feature I proposed above. This has taken a while, but has resulted in many cleanups all over the code base (check out the linked pull requests above this comment for details). I'm now homing in on a design that is pretty similar to the original derive idea (which has been surprising to me, given how big of a detour I took from that while prototyping), except a bit simpler. The design feels pretty finalized to me, but I have yet to finish the presentation layer code that is going to use it, so problems could still arise. If all goes well, I should have a pull request pretty soon.

Over the course of this process, my thinking on layers and what they need to look like has become more concrete. This led to the realization that the existing services code was already pretty similar to what we needed , so I adapted that and renamed it to layers (#2212). I've been refining the architecture further over several follow-up pull requests (also linked above), and things are more and more ready for the introduction of a presentation layer.

So while I'm not quite finished, it feels like I'm getting pretty close. Unforeseen problem are of course possible, as always.

@hannobraun
Copy link
Owner Author

I'm almost done! For the most part, things have just been falling into place since my update yesterday. There are still some vestiges where the color stored in Region is used, instead of getting it from the presentation layer, but rooting those out shouldn't be difficult now.

Unfortunately, I've run out of time for today (and likely this week), so getting this over the finish line will have to wait for a bit.

@hannobraun
Copy link
Owner Author

I'm still almost done. The current hold-up is that I've found a last-minute bug in my final pull request, and I've not had enough time for this over the last few days to get to the bottom of it.

I'll stay on it.

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