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

Revamp CSG functions, make curve canvases consistent & more #234

Merged
merged 43 commits into from
Sep 12, 2023

Conversation

Cloud7050
Copy link
Contributor

@Cloud7050 Cloud7050 commented Sep 6, 2023

The changes in this PR are described below.

CSG

Changes in: package.json, src/bundles/csg/, src/common/utilities.ts, src/tabs/Csg/, yarn.lock.

  • Revamp CSG exports
    • Add new ability to ungroup() Groups while applying accumulated transforms
    • Change centring of some primitives for better usability and consistency, instead of aligning all min bounds to origin, as not all primitives have full 1x1x1 bounding box dimensions. Closes Tweak primitive alignments Cloud7050/modules#30
      • Geodesic sphere & star now centred at (0.5, 0.5, 0.5) instead of aligning to origin
      • Square pyramid now has base length 1 with sides aligned to axes. No longer rotated 45° with corners touching 1x1 box by way of low resolution cone generator
      • Prism now centred instead of aligning to origin. Flat side now aligned with y axis instead of x
      • Torus now centred higher at (0.5, 0.5, 0.5). Ring is now slightly thicker, while maintaining 1x1 dimensions
    • Rewrite all user documentation. Fix various outdated/wrong information. Add significantly more detail. Closes Transformation/centre changes Cloud7050/modules#33
    • Officially support rendering directly passed Shapes as opposed to only Groups
    • Add more parameter checks to avoid likely user pitfalls and reduce unhelpful error messages. Enhance existing error messages and increase specificity
    • Rename shape_to_stl() to download_shape_stl() as it triggers a file download
    • Rename V2 Entity to Operable to avoid ambiguity with renderer and JSCAD internal entities. Operables are now a user-facing term
  • Finally upgrade to real BlueprintJS tooltips now that we have the technology. Fixes buggy tooltip positions that were meant to be a temporary measure. Tweak control hint text. Closes True BlueprintJS tooltips + new layout Cloud7050/modules#22
  • Upgrade to real js-slang List types and related functions now that we have the technology
  • Split samples into their own files. Add many new samples. Update existing samples to improve them and make them work with current CSG. Closes [CSG]: include Sierpinsky example in samples #231
  • Refactor, simplify, and move implementations, such as for primitives and utilities
    • Lists now only used in main functions file, as importing a file that works with real Lists from the tab end causes build failures. Add note regarding limitation. Groups now store arrays instead of Lists
    • Remove Entity cloning
    • Remove unused code
  • Move relevant CSS constants into their own common file. Update them for BlueprintJS V4
  • Lock @jscad/modeling version to match patch file exactly. Also remove unused lodash & @types/lodash
  • Remove commented references to fork issues. Now all resolved or transferred to main repo
  • Change default renderer colour

Curve

Changes in: src/bundles/curve/, src/tabs/Curve/, src/tabs/Rune/, src/tabs/common/ src/tabs/physics_2d/DebugDrawCanvas.tsx.

Build

Changes in: scripts/.

  • Fix yarn scripts build modules not building any tabs (used in yarn dev)
  • Change retrieveBundlesAndTabs() to clarify returned bundle/tab logic based on user input, and update jest test to match
  • Documentation & refactoring

Rune

Changes in: src/bundles/rune/rune_ops.ts/.

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.
Revise/clarify the logic of retrieveBundlesAndTabs() and update tests/comment.
Left over from tests with explicit tsconfig path when troubleshooting
build errors
Rename params, non-deep children copy for new Group
Copy link
Contributor

@sayomaki sayomaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is just some of my initial comments as I have not looked thoroughly at the CSG-related code and refactor yet (not my forte), but for the HTML/React related code it is looking pretty good to me although I do have a few small comments on that.

I have not yet tested these changes locally yet, but I will let you know if I find any problems.

src/tabs/Csg/canvas_holder.tsx Show resolved Hide resolved
src/tabs/Curve/animation_canvas_3d_curve.tsx Show resolved Hide resolved
@Cloud7050
Copy link
Contributor Author

@sayomaki @leeyi45 do you have any further comments for proceeding with the PR?

@leeyi45
Copy link
Contributor

leeyi45 commented Sep 9, 2023

@sayomaki @leeyi45 do you have any further comments for proceeding with the PR?

Nope, everything on my side is good. More improvements are yet to be made, but that'll be on my side.

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

A lot of files (especially newly-created and renamed ones) have incorrect line-endings, triggering ESLint warnings.

Could you correct them please? Thanks!

src/bundles/csg/jscad/renderer.ts Outdated Show resolved Hide resolved
@Cloud7050
Copy link
Contributor Author

The TypeDoc used to talk about reflections being a matter of scaling with negative factors. I have found in #206 that JSCAD doesn't like this, and Prof Low's decision regarding it. Therefore I am once again disallowing negative factors in scale().

@Cloud7050
Copy link
Contributor Author

Cloud7050 commented Sep 11, 2023

image

A lot of files (especially newly-created and renamed ones) have incorrect line-endings, triggering ESLint warnings.

Could you correct them please? Thanks!

I seem to be encountering the same hiccup as in #204. Actually the files are CRLF in my working dir, but looks like git is converting them to LF due to autocrlf.

To my understanding, the current LF/CRLF in the index is inconsistent. I would also like to accomodate different eol checkouts depending on OS, which the current eslint will complain about locally.

I was in the middle of some eslint changes in another branch that was for #136, so I didn't touch eslint here. There are actually some changes as well as fixes I wish to make to the eslintrcs (and probably gitattributes per #209), wider sweeping than in the draft of #241. Iirc eslint is currently operating wrongly in a few ways.

@RichDom2185 Do you think I could take over with my own tailored PR wrt eslint? I will note the existing changes you have made and try to incorporate them. There are reasons behind some of the choices made in the current config which could be further discussed in such a PR, eg the reason behind so many commented rules and options.

As for this PR, I would like to proceed knowing the checks are not happy with the eol, and address them in one swoop in the aforementioned PR.

They were called components when the params were clamped between 0-1.
Now more fitting to call them values when 0-255.
@RichDom2185
Copy link
Member

I would also like to accomodate different eol checkouts depending on OS

I agree with this. I'm not sure why our ESLint config enforces a specific line ending when git normally auto-adapts line endings when cloning to different OSses.

Do you think I could take over with my own tailored PR wrt eslint?

Sure, thanks. So far, all the changes in #241 are backwards-compatible so feel free to approve it if you me to merge it first (I'll edit the PR description accordingly).

As for this PR, I would like to proceed knowing the checks are not happy with the eol, and address them in one swoop in the aforementioned PR.

Ok, I'll review the rest of the files, ignoring these warnings for now.

@leeyi45
Copy link
Contributor

leeyi45 commented Sep 12, 2023

If this PR is ready for merging I'd like to go ahead with it, as there's another long line of changes that I need to merge too

@martin-henz
Copy link
Member

Looks like everyone is ok with merging it so I'm merging it.

@martin-henz martin-henz merged commit 87dcf69 into source-academy:master Sep 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants