search

jeffeb3 / sandify

web based user interface to create patterns that could be useful for robots that draw in sand with ball bearings.
MIT License
187 stars 34 forks source link

Groups restructure #262

Closed jeffeb3 closed 10 months ago

jeffeb3 commented 1 year ago

@bobnik, Feel free to point out anything you want to talk about. This still has a lot of stuff broken.

bobnik commented 1 year ago

Sorry my response has been so delayed - January was rough work-wise. I know this is a work-in-progress, so some of my comments may not ultimately be relevant.

It's probably best for me to wait until you complete your changes and stabilize the branch, and we can regroup after that?

jeffeb3 commented 1 year ago

Sorry my response has been so delayed

No worries. I need to get back into this. I have lost a little familiarity already as I have gone back to work.

Why did the layerSlice implementation need to change (removed parentId for effects, broke persisted state logic, etc)?

I think this was the whole point? The layerSlice didn't match what we wanted to be able to do with groups and this entire restructure.

Effects are "owned" by the layer, not the shape. There is one big 'byId' list of them, but that is only for redux optimization. Each layer has a list of effect ids, and the ids don't need to know their parents. The persistent state logic was broken just because it was not in the MVP and I was making breaking changes. It should be easy to fix now. We also wanted to be able to save/load state, so I'm not sure persistent state is done the way we will want it, so I just removed it.

Why can't Shape just be a Layer

Layers have one kernel shape. The layer has a type, which helps us figure out which shape to use. Shapes don't have effects. Shapes don't have origins, or scale, they are just kernels. Why can't Shape be it's own class, separate from Layer?

Morph (currently) feels extraneous

I talked a bit about morph in the other comment. I am not sure what part feels extraneous. But I am happy to consider it again with you.

jeffeb3 commented 1 year ago

The renamed selector variables and methods seem like a personal choice, and are going to take some getting used to.

It felt very messy and confusing to me before. Sorry if I broke it from you. We don't have the same brain, so there is going to be some compromise. Maybe just some more comments would help, or some refinement (like orderedLayerIds) would let us both grok it right away. I was working in a vacuum a little.

Some of the most confusing things to me were the fact that we have a Circle.js, which contains code to take a json object and return a vector of points. But the state saved in the redux state isn't a Circle object, it is just the json object with the options. My brain is not used to that at all. So I was trying to make it more clear when we had the model class, and when we had the json object that describes the options. Most of the movement was to make that clearer (to me, anyway).

I don't want to spend all our time arguing over the names of things. But it does matter a lot in this project. I just don't look at the code often. So when I return, I need something that makes sense. Maybe some more comments would help. Or a cheatsheet/glossary for me.

I'm not sure if the difference is mostly because I don't work in Javascript/redux/react on anything else, or if it just because we don't work together often enough. We ought to be able to find some names that we both agree on.

bobnik commented 1 year ago

RE: your various comments: makes sense. For now, I'll stay out of the code base. Let me know when you want me to look again.

bobnik commented 10 months ago

This is obsolete, per #275