OpenJSCAD.org
OpenJSCAD.org copied to clipboard
refactor(modeling)!: no geometry clone
This PR removes geometries *.clone()
functions.
Geometries are immutable, so there is no need for a clone function to be available to users.
The purpose of clone
was already unclear. It does a shallow clone of the geometry object. It does not do a deep clone of points. So even with a clone you need to be careful not to mutate parts of the data structure.
There are only three places where geometry data structures are mutated:
-
colorize
-
transform
-
reverse
These are all internal uses of clone
, and I replaced them with calls to Object.assign({}, geometry)
. Colorize is the only one that mutates a geometry from outside the geometries
subpackage. I left it there for now though.
I did not remove the maths *.clone()
functions, because vectors are mutable. Any function with an out
parameter like vec3.min
and vec2.copy
all mutate the data structure. So I left clone in the maths.
Maybe this will be controversial? I think it's a good change. Simplifies the API, and reduces the number of unnecessary allocations. Let me know what you think.
All Submissions:
- [x] Have you followed the guidelines in our Contributing document?
- [x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
- [x] Does your submission pass tests?
Simplifies the API, and reduces the number of unnecessary allocations. Let me know what you think.
:clap:
To make the case stronger, is there a measurable performance impact?
We should not leave mutating stuff, especially transform and colorize. They can simply with assign copy geometry data and produce a new object with changed color or transform. The biggest data is reused in this case, so there is no need for mutability of color or transform.
Nope. These data structures should be treated as objects, which means "encapsulation".
some of the changes are good, so decide now..
I was making this case previously, and efforts were made in that direction for jscad not to mutate for transform and colorize. I am for gong in the direction of immutability after something is produced. It is fine to use builders or such, but then builders should create immutable objects that can be reused.
I should clarify that colorize
, transform
and reverse
work by first cloning the given geometry, and then mutating the cloned object. From a user point of view, it does NOT mutate the input geometry.
I think it is okay for us to INTERNALLY mutate objects. So for performance reasons, I think it is fine for us to use mutable vectors, as long as we never mutate a vector passed in by a user. As a general rule:
If you modify state, it should only be on an object you "own"
Another example which I think is okay... the way we do applyTransforms
modifies the state of an object. Since javascript is single threaded, it does this "atomically" and replaces the transformed points at the same time as resetting the transform to the identity.
I don't actually think these behaviors should change. I only mentioned them here since cloning and mutability are linked concepts. And those three places were the only ones that used clone
and also mutated state.
I am not sure of current state of applytransform. It is OK to replace the internal state in that way, but not ok to change contents of the original points array as other instances will have reference to it. Sry, can't see code, typing from phone.
I took a look at transform code, and from what I have seen transform will not mutate points array itself in-place, but create a new, thus others using same points array will not be affected. So conceptually immutability is respected.
Nope. This is not going to happen as provided. I'll provide more reasoning tomorrow.
OK... about the geometries (geom2, geom3, path2)...
although these are currently a set of functions for manipulating an anonymous object, these are really 'objects', and should be treated as such. (and who knows... someday these might become Javascript objects)
if these were objects then a base class might be created to provide some basic 'geometry' functions, like constructors that take an array, clone, transform, applyTransforms, etc. But this isn't possible today, so there's some duplication.
In addition, the data within the objects would be 'protected' from direct access (Although Javascript objects do not have this concept yet)
again, the geometries should be treated as objects, which means there's both data and interface. the interface makes sure that the data is encapsulated/protected, and also makes sure that data is exposed in the requested format, i.e. polygons vs points vs etc, etc.
by doing this, the data structure and/or logic can change without impacting functions that use the geometry.
for example, the recent changes to geom2 data structure didn't take a lot of logic rewrites because the internals correctly called the geometry functions.
yeah sure. internals can cheat, access the data structures directly, and even manipulate data. but any change to data structures has a hug impact on logic, creates bugs, etc.
but what about performance? well... a single function call is not going to create a performance bottle neck. allocations of any kind have a huge performance impact.
really, the best way to improve performance going forward is finding a better data structure, and couple the data together with improved logic.
Totally agree with your points about encapsulation. It did make it nice and easy to refactor the geometry internals without breaking too much.
There is a question of which functions exactly are "allowed" to access the internals of a data structure. I think the ideal would be that internals can ONLY be accessed by their specific geometry package. So I would say that anything in the geom2
directory should be allowed to access internals directly of a geom2
object. Anything outside geom2
directory should have to use access functions like toOutlines
.
That's why I mentioned in the PR that colorize
seems to violate this separation. There might be other places too.
There is a question of which functions exactly are "allowed" to access the internals of a data structure. I think the ideal would be that internals can ONLY be accessed by their specific geometry package. So I would say that anything in the
geom2
directory should be allowed to access internals directly of ageom2
object. Anything outsidegeom2
directory should have to use access functions liketoOutlines
.
Hmm... I'm wondering if we should put these tidbits into handbook for developers. I'm sure there are plenty to document.
That's why I mentioned in the PR that
colorize
seems to violate this separation. There might be other places too.
Actually, the IO packages set/use other attributes of the geometries.
V1 had the concept of a 'shared' attribute which held the color. It was a dumb name, but the concept might be useful. Maybe we could resurrect this idea as 'properties'. (SVG and 3MF have his concept) There are all kinds of cool things that could be supported, even textures.
I am looking this thing with immutability from the perspective of specific optimization for displaying geometry in WEBGL, being able to recognize a geometry that is in multiple places, or a geometry that did not change internally between parameter changes.
I agree that transform and color and other possible attributes are better kept separated from geometry, that way we do not need to implement these common things for each geometry, and supporting more attributes is easier.
Thinking back to boolean operations, I could imagine a boolean operation that produces a group of geometries that together describe a solid (when vertives are combined) but keep reference to attributes of the originating objects that were used for the boolean. Than way you could display line number of where a part of boolean was created, and preview the original object. If you point to a hole you could preview the cylinder used to drill it. Also you could keep color and texture of each section without resorint to colorPerVertex.
Also I want to be able to support more than one representation of a 3d solid, that can then be converted to common representations for display, export, or prep for boolean. It is more efficient to import a STL mesh in as set of vertices than converting to polygons. that representation is fine for webgl and stle export, and for boolean is trivial to convert. Another thing is that there are boolean engines that work with mesh internally anyway(manifold).
btw @z3dev JavaScript has support for private fields in classes: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields
My main goal currently is to improve development tooling, so my thinking is in that direction, and performance is an important part of better dev experience.
One huge thing I a have seen is that JSX has widespread tooling support for code transformers to enhance to JSX function calls to inject file and line information, that way you can in dev build tract every DOM node to a line of code in the source. I am thinking of intercepting require('@jscad/modeling') to replace with a wrapper package where jscad functions have and additional parameter fit source info subtract({file:'main.js', line:23, col:4}, whatever was originally in the source)
, to be decided if ever made is if the injected param is first or last
I am looking this thing with immutability from the perspective of specific optimization for displaying geometry in WEBGL, being able to recognize a geometry that is in multiple places, or a geometry that did not change internally between parameter changes.
we could revisit the vtree package again. @platypii what do you think?
I agree that transform and color and other possible attributes are better kept separated from geometry, that way we do not need to implement these common things for each geometry, and supporting more attributes is easier.
Yeah. There's nothing easy about good designs. @platypii is suggesting some pretty major changes... all good to think about... so obviously there's some pretty cool stuff coming. I would really like to replace the 3D booleans with something a lot more modern.
Thinking back to boolean operations, I could imagine a boolean operation that produces a group of geometries that together describe a solid (when vertives are combined) but keep reference to attributes of the originating objects that were used for the boolean. Than way you could display line number of where a part of boolean was created, and preview the original object. If you point to a hole you could preview the cylinder used to drill it. Also you could keep color and texture of each section without resorint to colorPerVertex.
WOW! That's a huge step forward but maybe there's a few improvements which can support this functionality.
My main goal currently is to improve development tooling, so my thinking is in that direction, and performance is an important part of better dev experience.
Super. How about opening a discussion about some better division of packages. Anything related to rendering and website development would help to reorganize the 'core'.
I would really like to replace the 3D booleans with something a lot more modern.
Actually, booleans are one of useful ways to model stuff, but can have big impact with performance. Fortunately, there are options to make it much faster in the wild, so to some extent booleans will be fine to use I think in the future too.
Instead of replacing, I would go about it by adding more options for generating the models like BREP. More ways to create 2D models to later extrude, and more ways to construct 3d models other than boolean ops.
The direction I would like to see jscad to go is: anything JavaScript can run to create models
which means opening to running even WASM stuff like OCCT or even openScad. That is why I like that long time ago jscad decided to in direction of npm modules, and now ESM support too. I love JavaScript and I love modelling 3D in JavaScript, and would love to use anything that JavaScript can run under umbrella of jscad.
@z3dev I updated the docs as suggested, and responded to your review comments. Let me know if there are changes you'd like made, or changes you would like reverted. Most of the "ditto" comments are addressed here
@z3dev I updated the docs as suggested, and responded to your review comments. Let me know if there are changes you'd like made, or changes you would like reverted.
I'm not a fan of removing the clone() functions, so please revert. I think using the Object functions just makes the code less understandable. And of course, it's kind of violating encapsulation in many ways. It's kind of like casting a pointer in C++ from derived class to base class, which is not a good idea.
I'm not a fan of removing the clone() functions, so please revert. I think using the Object functions just makes the code less understandable. And of course, it's kind of violating encapsulation in many ways. It's kind of like casting a pointer in C++ from derived class to base class, which is not a good idea.
I disagree that it is violating encapsulation. But I agree that having a clone function is more clear than using Object.assign.
What do you think about making clone()
a private function which is not exposed to users? So basically remove it from index.js
and only using it internally? That would seem to be better encapsulation, while still making the code easy to understand?
I'm not a fan of removing the clone() functions, so please revert. I think using the Object functions just makes the code less understandable. And of course, it's kind of violating encapsulation in many ways. It's kind of like casting a pointer in C++ from derived class to base class, which is not a good idea.
I disagree that it is violating encapsulation. But I agree that having a clone function is more clear than using Object.assign.
What do you think about making
clone()
a private function which is not exposed to users? So basically remove it fromindex.js
and only using it internally? That would seem to be better encapsulation, while still making the code easy to understand?
Sure. If that's possible then let's do that. applyTransforms() can also be private... i think.
@platypii any questions?
i added a few new issues to track some of the isssues / ideas discussed here.
#1195 #1204 #1205
I've been thinking about it still, but if we don't remove the clone functions, then 1) there is not much left of this PR, 2) if they are made "private", then we need to figure out another way to solve the "colorize" problem. #1205 might help with that.
I still like the idea of removing clone, personally. But if we're not doing that I might just close this PR. For now I want to continue thinking about it though, and trying some code options.
I'm not a fan of removing the clone() functions, so please revert. I think using the Object functions just makes the code less understandable. And of course, it's kind of violating encapsulation in many ways. It's kind of like casting a pointer in C++ from derived class to base class, which is not a good idea.
If this is the case, I would argue that then the current way clone is implemented is definitely wrong. We have an external function that does cloning but talking about encapsulation. If you want to support clone, then it is best to have the clone be a method of the encapsulated object. Also in that case if clone method is not present we can fall-back to Object.assign.
To me, this mix of bunch of functions in each geometry type with xxx.isA() functions gives worst of both worlds Object oriented and functional.
But I agree that having a clone function is more clear than using Object.assign.
In that case we need to expose a SINGLE clone function with clear definition what it does, and internally we can change how it is implemented without braking the code. Clone function is useful to the users, not just internally. It should call .clone() and fallback to Object.assign.
For a CSG tree (vtree) we need objects that carry information, and for interlan computations, we can do whatever, and the goal is to simplify converting from one to another taking care of performance. Geometries being immutable should have an unique identifier so they can be more easily reused if you for example export them to vtree, and then do comupting and recomputing . When geometry data is crossing barriers like postMessage
we should be able to recognize reused geometry without inspecting each vertex.
I've been thinking about it still, but if we don't remove the clone functions, then 1) there is not much left of this PR, 2) if they are made "private", then we need to figure out another way to solve the "colorize" problem. #1205 might help with that.
Actually, there are lots of non-related changes. If this was just a PR to remove clone then someone may make heads of tails.
I'm not sure why there were comment changes, utility function changes, etc.
Maybe another PR would be better.
Actually, there are lots of non-related changes. If this was just a PR to remove clone then someone may make heads of tails.
I'm not sure why there were comment changes, utility function changes, etc.
All the changes here are related to removing clone
. The comment changes were things like:
- * @returns {geom2|geom3} a new geometry
+ * @returns {geom2|geom3} a geometry
Because it technically no longer returns a "new" geometry in some cases.
However, I rebased without the comment changes. I can bring them back later, but I would rather focus on the content of the code changes. This makes the PR cleaner.
I still think this is a good change: to avoid unnecessary allocations, be more consistent about immutability, and simplify the API. But please let me know your thoughts @z3dev
I disagree that it is violating encapsulation. But I agree that having a clone function is more clear than using Object.assign.
Sure. If that's possible then let's do that. applyTransforms() can also be private... i think.
How about hiding applyTransforms?
How about hiding applyTransforms?
I think it is still useful for some end users, as it gives access to mesh data that is with applied transformation-
Is this better? const cloned = Object.assign({}, geometry)
With immutability respected, cloning is not needed for end users I think. I may be missing the point of this :)
Let's try this again...
- Geometries are not JS classes so private method isn't possible
- Clone only does shallow copies
- Geometries are immutable, even applyTramsforms() respects this
- Clone only does shallow copies
- Geometries are immutable
Did I miss something?
I see the options as...
- Prefer object.assign() to clone (this is still shallow copies)
- Remove clone() from indexes (make clone private / internal only)
- Create a general internal clone() utility that does... shallow copies using object.assign()
- Convert geometries to JS classes, and make clone() private
It makes sense to keep encapsulation, meaning only functions defined within the geometry module can access the internals. So, clone() probably needs to remain, but hidden / private / internal only.
color() is another problem, and seems to be the design flaw. The concept should be that properties (color, texture, name, id, etc) become part of the geometry, and remain even after transformations are applied. But there could be many large properties assigned to each geometry, so the properties need to be shared across geometries to reduce allocations.
maybe we should correct the properties first. If the use of shared properties corrects color(), i.e. no more use of clone, then further improvements may become apparent.
color() is another problem, and seems to be the design flaw. The concept should be that properties (color, texture, name, id, etc) become part of the geometry, and remain even after transformations are applied. But there could be many large properties assigned to each geometry, so the properties need to be shared across geometries to reduce allocations.
maybe we should correct the properties first. If the use of shared properties corrects color(), i.e. no more use of clone, then further improvements may become apparent.
I would LOVE it if we moved color
to a general metadata property, something like:
{
"polygons": [],
"metadata": {
"color": [1, 0, 0, 1],
"reflectivity": 0.2,
"material": "abs plastic",
"texture": "texture.jpg",
"tolerance": 0.001,
}
}
Then have general rules about how metadata is preserved through operations, not just color. Renderers could decide what information to use or not.
But there could be many large properties assigned to each geometry, so the properties need to be shared across geometries to reduce allocations.
with immutable principle and shallow copies, this is fine, only references to large properties are copied.
transform and color are fine like they are if they use shallow copy and modify only their respected properties.
if you define transform as function that
- creates shallow copy
- uses existing transform and combines with the new
- replaces only transform property on the new object
if you define transform as function that
- creates shallow copy
- replaces only color property on the new object
they do not need to know anything about the format, except that we reserved transforms
and color
for them to play with.
colorize defined like this would return a new object that has same geometry and transform and everything , and a new color.
applyTransform
even though the resulting object is the same, a copy should be made where new object has the new geometry with transform applied, and identity matrix, and all other properties.