refactor(modeling)!: no geometry clone #1201
Conversation
👏 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 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:
Another example which I think is okay... the way we do 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 |
|
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. |
| @@ -18,6 +18,6 @@ import * as vec3 from '../maths/vec3/index.js' | |||
| * @example | |||
| * let myconnector = create() | |||
| */ | |||
| export const create = () => ({ point: vec3.create(), axis: vec3.clone([0, 0, 1]), normal: vec3.clone([1, 0, 0]) }) | |||
There was a problem hiding this comment.
Well... the logic in connectors is going to use vec3 functions so the axis and normal has best be vec3
There was a problem hiding this comment.
But the vec3.clone function assumes that it takes in a vec3 and returns a vec3. So what exactly was this clone accomplishing? If the input is not a valid vec3, then how is cloning going to help? Ditto for many of the comments below.
There was a problem hiding this comment.
Understood. Then I suggest using fromValues()
There was a problem hiding this comment.
Agreed. The cloning doesn't help.
But the underlying NUMERIC type might help, so I suggested using fromValues()
| @@ -14,7 +14,7 @@ export const fromCompactBinary = (data) => { | |||
|
|
|||
| const created = create() | |||
|
|
|||
| created.transforms = mat4.clone(data.slice(1, 17)) | |||
| created.transforms = data.slice(1, 17) | |||
| @@ -16,7 +16,7 @@ export const fromCompactBinary = (data) => { | |||
|
|
|||
| const created = create() | |||
|
|
|||
| created.transforms = mat4.clone(data.slice(1, 17)) | |||
| created.transforms = data.slice(1, 17) | |||
| @@ -14,7 +14,7 @@ export const fromCompactBinary = (data) => { | |||
|
|
|||
| const created = create() | |||
|
|
|||
| created.transforms = mat4.clone(data.slice(1, 17)) | |||
| let baseSlice = slice.fromGeom2(geometry) | ||
| if (offsetv[2] < 0) baseSlice = slice.reverse(baseSlice) | ||
| if (offset[2] < 0) baseSlice = slice.reverse(baseSlice) |
There was a problem hiding this comment.
This looks fine but i think this whole function needs some changes to improve performance. This function is not exposed so all parameters should be checked by calling functions, and passed to extrudeGeom2 with the correct type. Also extrudeGeom2 should be renamed to match the file name. etc. etc.
There was a problem hiding this comment.
I'm happy to make that change in another PR
There was a problem hiding this comment.
Cool. How about a new issue to track?
|
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 That's why I mentioned in the PR that |
Hmm... I'm wondering if we should put these tidbits into handbook for developers. I'm sure there are plenty to document.
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 |
we could revisit the vtree package again. @platypii what do you think?
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.
WOW! That's a huge step forward but maybe there's a few improvements which can support this functionality.
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'. |
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: |
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 |
Sure. If that's possible then let's do that. applyTransforms() can also be private... i think. |
|
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. |
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.
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 |
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. |
All the changes here are related to removing - * @returns {geom2|geom3} a new geometry
+ * @returns {geom2|geom3} a geometryBecause 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 |
There was a problem hiding this comment.
@platypii I just have a few comments. Please review.
Overall, I don't mind the changes. It really hasn't changed the logic or the readability.
But I'm a developer so these changes to API don't mean much. But what about the users who are trying to make designs?
Is this better?
const cloned = Object.assign({}, geometry)
And how do you document that little tidbit?
| @@ -18,6 +18,6 @@ import * as vec3 from '../maths/vec3/index.js' | |||
| * @example | |||
| * let myconnector = create() | |||
| */ | |||
| export const create = () => ({ point: vec3.create(), axis: vec3.clone([0, 0, 1]), normal: vec3.clone([1, 0, 0]) }) | |||
There was a problem hiding this comment.
Agreed. The cloning doesn't help.
But the underlying NUMERIC type might help, so I suggested using fromValues()
| @@ -36,11 +36,10 @@ export const appendArc = (options, geometry) => { | |||
|
|
|||
| // validate the given options | |||
| if (!Array.isArray(endpoint)) throw new Error('endpoint must be an array of X and Y values') | |||
| if (endpoint.length < 2) throw new Error('endpoint must contain X and Y values') | |||
| let baseSlice = slice.fromGeom2(geometry) | ||
| if (offsetv[2] < 0) baseSlice = slice.reverse(baseSlice) | ||
| if (offset[2] < 0) baseSlice = slice.reverse(baseSlice) |
There was a problem hiding this comment.
Cool. How about a new issue to track?
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- |
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...
Did I miss something? I see the options as...
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. |
I would LOVE it if we moved {
"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. |
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
if you define transform as function that
they do not need to know anything about the format, except that we reserved colorize defined like this would return a new object that has same geometry and transform and everything , and a new color. applyTransformeven 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. |
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
clonewas 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:
colorizetransformreverseThese are all internal uses of
clone, and I replaced them with calls toObject.assign({}, geometry). Colorize is the only one that mutates a geometry from outside thegeometriessubpackage. I left it there for now though.I did not remove the maths
*.clone()functions, because vectors are mutable. Any function with anoutparameter likevec3.minandvec2.copyall 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: