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

feat(modeling): preserve color for offset and extrude #1275

Merged
merged 3 commits into from
Aug 13, 2023

Conversation

platypii
Copy link
Contributor

@platypii platypii commented Aug 10, 2023

This PR preserves the color attribute when applying operations like offset and extrude and project.

This seems like a clear improvement for users of JSCAD. If you have a 2D object with color like on the left, would you expect extrusion to preserve the color? Or lose it?

extrudeColor

preserve-color.js
import * as jscad from '@jscad/modeling'
const { colorize } = jscad.colors
const { extrudeLinear } = jscad.extrusions
const { square } = jscad.primitives
const { translate } = jscad.transforms

export const main = () => {
  const obj2d = [
    colorize([1, 0, 0], square({ center: [0, -4] })),
    colorize([0, 1, 0], square({ center: [0, 0] })),
    colorize([0, 0, 1], square({ center: [0, 4] })),
  ]
  return [
    translate([-4, 0], obj2d),
    translate([4, 0, 0], extrudeLinear({}, obj2d)),
  ]
}

If someone really want no color, it's easy to remove using colorize. But if you want to preserve color of sub-components through an operation... there's really no good way to do it without this change.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission pass tests?

@platypii platypii requested review from hrgdavor and z3dev August 10, 2023 04:03
hrgdavor
hrgdavor previously approved these changes Aug 10, 2023
Copy link
Contributor

@hrgdavor hrgdavor left a comment

Choose a reason for hiding this comment

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

This a move in right direction. Colors from boolean ops were removed for perf gain, but it is possible we will be able to make keeping colors optional in the future for booleans.

Copy link
Member

@z3dev z3dev left a comment

Choose a reason for hiding this comment

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

@platypii nice set of changes, and very nice improvements for the user experience.

i just have a bunch of nits. sorry.

@platypii
Copy link
Contributor Author

@z3dev I just pushed some more changes which I hope will address your concerns:

  • returns new geometries even if the shape is the same
  • handles empty geometries when it makes sense
  • added tests for the above
  • addressed other PR feedback (function and variable names)

Let me know what you think

Copy link
Member

@z3dev z3dev left a comment

Choose a reason for hiding this comment

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

@platypii thanks for hanging in there. Sometimes it take time to sort out differences in opinion.

The changes are all good now, with improvements everywhere.

@z3dev z3dev merged commit 48606fc into jscad:V3 Aug 13, 2023
@platypii platypii deleted the V3-preserve-color branch August 13, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants