CSG Module 2.0 #206
CSG Module 2.0 #206
Conversation
… rotate_x/y/z and scale_x/y/z in functions.ts.
- Refactored Group class to return a new Group consisting of a shallow copy of its children for every transformation - Removed several functions in functions.ts to simplify the csg module
…() and various renderer funtions.
.idea/.gitignore
Outdated
There was a problem hiding this comment.
I think the files in the .idea folder may have been mistakenly committed into your branch. Do you mind deleting them?
|
What's the status of this PR? Merge? |
|
It is still under development as we still have several bugs to resolve. Now that finals has ended, I will work on it with Chenbo. |
Ok, in that case, let us mark it as draft first. Feel free to mark the PR as ready when the bugs have been resolved! |
joeng03
force-pushed
the
csg
branch
2 times, most recently
from
dc81e48
to
130729b
Compare
…on on transformed Boolean operation product' bug
|
Hi @RichDom2185, we have resolved the bugs and are ready to merge! |
Do you have some demo/example code to play around with this module? |
src/bundles/csg/functions.ts
Outdated
| function colorize(shape: Shape, hex: string) { | ||
| let color: Color = hexToColor(hex); | ||
| let coloredSolid: Solid = _colorize(color, shape.solid); | ||
| return new Shape(coloredSolid); | ||
| } |
There was a problem hiding this comment.
Just like the workflow warning says, this function does not seem to be used anywhere?
There was a problem hiding this comment.
Was there any reason functions like
scale_(x|y|z),flip_(x|y|z),transform_(x|y|z),shape_center,area,volume, etc. were removed from the list of exports?
It was a change requested by Prof Low to make the set of exported functions less redundant
Just like the workflow warning says, this function does not seem to be used anywhere?
Thanks for reminding :) Going to remove it in the latest commit
| * ranging from 0 to infinity. | ||
| * For example scaling the shape by 1 in x, y and z direction results in | ||
| * Scales the shape in the x, y and z direction with the specified factor. | ||
| * Factors must be non-zero. |
There was a problem hiding this comment.
Why must factors be non-zero? Strictly speaking, shouldn't it be technically possible to scale an entity by a factor of 0 (making it infinitesimally small)?
If it is to get around an error, I don't think this is the best way to do it.
There was a problem hiding this comment.
The JSCAD code (node_modules@jscad\modeling\src\operations\transforms\scale.js) currently does not even allow negative values and will throw an error if factors are <=0. We decided to add a <=0 check in function.ts to avoid revealing the JSCAD implementation
There was a problem hiding this comment.
This sounds like we are artificially enforcing that factors are positive simply to get around this limitation/error of the JSCAD library. It's not about revealing implementation; what I meant was more of a question of "should we support scaling by non-positive factors?".
If we can define scaling by a negative number to be _"flip in the corresponding axes that are scaled by a negative factor, and then scale by the absolute value of the factors" then why can't we just implement it that way in the code? Artificially capping it to >= 0 just because it will through an error otherwise does not seem like the best thing to do. Is there any reason (perhaps pedagogical/etc.) that it was not done this way?
Likewise, for scaling with a 0 factor.
There was a problem hiding this comment.
While it is possible to support negative scaling by patching the JSCAD code, Prof has said that he does not support the inclusion of such a functionality.
"I don't support the inclusion of negative scaling. It is not often used in 3D modelling. Would rather have a separate flip function to do that. Since all the primitive shapes we provide have some symmetry such that flipping can be achieved with rotation, a separate flip function or negative scaling is not necessary."
There was a problem hiding this comment.
Okay sure, if that's the reason then I guess that's fine. Thanks for the clarification!
There was a problem hiding this comment.
What exactly is the command you are using to merge master into this branch? Your force pushes seem to be causing weird things in the GitHub PR list of changes.
There was a problem hiding this comment.
I will try to troubleshoot. Essentially, the module is done right? All that's left is to merge master?
There was a problem hiding this comment.
Yep, all that's left is to merge master
There was a problem hiding this comment.
Can you create a new branch with the contents of this PR just before your most recent force-pushed? (This one below)
What was the rationale of the force-push btw?
There was a problem hiding this comment.
Nevermind, was able to dig the commit out of GitHub's servers. See #213.
Its in samples.md, do let me know if you run into any issues with the sample code! |
|
Superseded by #213. |
|
Hi @RichDom2185 , thanks for solving the issues of this PR! |
Hi, refer to my comment here: #206 (comment). I honestly did not know what command you used, so I can't comment on that (yet). But what I did was:
Of course, this is harder than it sounds. In reality, the first 2 interactive rebase attempts were unsuccessful because of an incorrect ordering of commits (this is what happens when you try to rebase after you have merged some branches – generally, either use merge commits all the way or rebase all the way to make your life easier, don't use/mix both methods). The main issues were:
The above comment was mainly about the merge conflicts that I encountered while rebasing, and focused on how we can prevent conflicts (by configuring our editors properly). In terms of how I resolved it, it is no other than just a simple rebase on top of |
|
Based on the commit titles and hashes of this (and my experience debugging PRs), I strongly suspect somewhere along the way, the following happened:
|
* feat(src): Extract & update CSG's CSS constants * chore(csg): Remove V1 comments referencing fork issues * feat(csg): Migrate to BlueprintJS Tooltips * devfeat(csg): Update samples * devfeat(curve): Add samples * docs(curve): Update animation function docs * ref(csg): Remove default colour The addition of operation colour preservation and removal of the store/render pattern means that all Shapes now require an explicit colour. Changing the default colour of the wrapped renderer no longer has a visible effect. * fix(build): Fix `build modules` building no tabs Revise/clarify the logic of retrieveBundlesAndTabs() and update tests/comment. * feat(): Enhance & make curve canvases consistent * feat(csg): Replace hint dots with colons * ref(csg): Use real js-slang lists * devfeat(csg): Update colours sample * devfeat(csg): Add primitives sample * chore(pkg): Lock modeling version for patch file * ref(build): Reorder config to match Left over from tests with explicit tsconfig path when troubleshooting build errors * ref(csg): Rename to Operable, no List, move/del funcs * fix(csg): Fix primitive sample trailing comma * devfeat(csg): Add Prof's Sierpinski fractal * FEAT(csg): Overhaul functions, documentation * devfeat(csg): Update sample import names * ref(csg): Reorder sample functions * devfeat(csg): Add rotation sample * devfeat(csg): Add summary snowglobe example as sample * docs(csg): Plural categories, bulleted list, tweaks * ref(csg): Refactor operables Rename params, non-deep children copy for new Group * feat(csg): Improve error feedback for pitfalls * fix(csg): Implement proper ungroup * feat(csg): Improve error feedback for pitfalls * feat(csg): Update sample utility funcs * feat(csg): Update sierpinski sample for new primitive * fix(rune): Fix low resolution heart * docs(csg): Link to GitHub samples in summary * feat(csg): Update sierpinski sample * feat(csg): Readd custom default colour due to bug #227 * ref(build): Remove leftover from troubleshooting * docs(csg): Tweak pyramid dimensions description * feat(sound): Throw error on invalid sound durations Closes #110 * chore(pkg): Remove unused lodash * feat(curve): Add Source version to canvases sample * feat(tabs): Remove enforced spacing of MultiItemDisplay * ref(csg): Default colour back in constants * fix(csg): Disallow negative scaling, update docs instead Based on #206 (comment). * ref(csg): Rename RGB components to values They were called components when the params were clamped between 0-1. Now more fitting to call them values when 0-255.
Description
Objectives:
Statement:
Fix the current issue of incorrect display of colour after boolean operations (union, subtract, intersect) on primitive shapes. Currently, the colours of the initial Shapes are not preserved in the resulting Shape, instead a default colour (grey) is used.
Approach:
This can be achieved through patching the @jscad/modeling package. We have used patch-package to ensure that our patch is applied seamlessly every time a new installation (i.e. every run of yarn install) is performed. By tracing the code and referring to Version 1 of JSCAD and CSG.js, we found out that the colors are not preserved due to the single polygons which make up a solid not storing the color. The boolean operations are carried out through a Binary Space Partitioning (BSP) tree data structure, in which the color of polygons are not preserved during when they are split. We modified several methods to ensure that the colors of individual Shapes are stored in the polygons after boolean operations. Since retesellation combines coplanar planes even when they have different colors, we prefer to remove the retesellation functionality for this stage of development.
Statement:
Introduce hierarchy of Shapes by adding Groups. Groups can consist of Shapes or Groups. When transformations are applied to Groups, all objects in the Group undergo the transformation.
Approach:
A new class, Group was added to the utilities.ts file. Group takes in a List of Group|Shape, and contains an attribute, transforms, which stores the transformation matrix of the Group.
Both the Shape and Group classes now implement the Entity interface, which contains the transformation methods.
Statement:
Delay transformations on objects until the object is to be rendered to increase speed during modelling. Combine transformations when performing the final transformation, so that each individual Shape only undergoes one transformation operation. This feature has to integrate with Groups.
Approach:
Both the Shape and Group classes now implement the Entity interface, which contains the transformation methods. Due to considerations for efficiency, Groups actually shallow copies all its children under the hood during transformations, while creating an impression of immutability (new object created) on the client side. Deep copying is done lazily (delayed until rendering stage). During the rendering stage, the transformations are passed down the tree structure of the Group.
Statement:
Enforce the immutable and side-effect-free functional programming experience of the CSG module. This includes enforcing the immutability of shapes, the creation of a new object for every operation, and the removal of statefulness during rendering.
Approach:
Every primitive is now exported as a function which takes in a hex string as color, and further mutation of colors of a Shape throughout its lifecycle are now disallowed. Furthermore, the store() function is removed, and its functionality is now achieved by passing either a Group or a Shape into the render() function in the final lines of the code.
Statement:
Add a STL serializer to serialize CSG shapes into .stl files, for the purpose of 3D printing.
Approach:
Add another dependency, @jscad/stl-serializer, which exports a method, serialize, to convert a JSCAD shape into an .stl file.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Checklist: