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

Adding points in Line #181

Closed
jugglingcats opened this issue Nov 17, 2020 · 5 comments · Fixed by #247 or #259
Closed

Adding points in Line #181

jugglingcats opened this issue Nov 17, 2020 · 5 comments · Fixed by #247 or #259
Assignees
Labels
enhancement New feature or request released on @beta released in beta for review

Comments

@jugglingcats
Copy link

Hi, firstly thanks for the project!

I am trying to draw a line with width where the path changes based on state. The number of points to be drawn can increase or decrease as state changes.

It seems that the geometry in Line is not updated when the number of points change. I realise this might be related to the way three works and might be desirable from a performance perspective, but if I use the basic r3f line element, the line is updated (presumably by re-creating the geometry) as points are added.

I tried hacking around with onUpdate, similar to how it works with line, but it didn't work.

I created codesandbox showing the issue: https://codesandbox.io/s/r3f-line-adding-points-y7utc?file=/src/index.js

The Line is created with 5 points initially, so you can add up to five points with the Add Point button, but after that new points don't appear.

Any help/workaround much appreciated!

@jugglingcats
Copy link
Author

I created a workaround by copying the code in https://github.com/pmndrs/drei/blob/master/src/Line.tsx. Basically creating a new geometry when the points change.

My modified version looks like this (I stripped out some of the params we don't need):

  const TestLine = React.forwardRef(function Line({ points, color = 'black', lineWidth, ...rest }, ref) {
    const [line2] = useState(() => new Line2())
    const [lineMaterial] = useState(() => new LineMaterial())
    const [resolution] = useState(() => new Vector2(512, 512))
    useEffect(() => {
      const geom = new LineGeometry()
      geom.setPositions(points.flat())
      line2.geometry = geom
      line2.computeLineDistances()
    }, [points, line2])

    return (
      <primitive dispose={undefined} object={line2} ref={ref} {...rest}>
        <primitive
          dispose={undefined}
          object={lineMaterial}
          attach="material"
          color={color}
          resolution={resolution}
          linewidth={lineWidth}
          {...rest}
        />
      </primitive>
    )
  })

And in codesandbox... https://codesandbox.io/s/r3f-line-adding-points-workaround-11g9h?file=/src/index.js:1372-2184

In the codesandbox demo there is a perceptible flicker when clicking the buttons but I don't see this in my proper app, so it seems to work pretty well.

@mattrossman
Copy link
Contributor

Were you going to make a PR for this?

@joshuaellis joshuaellis added enhancement New feature or request good first issue Good for newcomers labels Jan 12, 2021
@joshuaellis
Copy link
Member

I've just had a look at this, I'm curious why it wouldn't update as the component is now. @mattrossman or @jugglingcats do you have any thoughts about this?

@mattrossman
Copy link
Contributor

@joshuaellis I believe there is no straightforward way to dynamically change the number of points in a line in Three.js (see https://stackoverflow.com/q/14840026/8371763), best you can do is pre-allocate a buffer with a maximum number of points and ignore the parts you don't use. Easiest solution seems to be just creating a new geometry as @jugglingcats does

@joshuaellis
Copy link
Member

I see, thanks for the insight! I'll work on implementing @jugglingcats' solution. I don't necessarily see an issue with it and it makes the component more "reactive"

@joshuaellis joshuaellis linked a pull request Jan 20, 2021 that will close this issue
@joshuaellis joshuaellis removed the good first issue Good for newcomers label Jan 20, 2021
joshuaellis added a commit that referenced this issue Jan 21, 2021
* feat: re-create geometry on new points

* refactor: useMemo the geometry instead of useEffect
@joshuaellis joshuaellis added the released on @beta released in beta for review label Jan 21, 2021
@joshuaellis joshuaellis self-assigned this Jan 21, 2021
@joshuaellis joshuaellis mentioned this issue Jan 26, 2021
10 tasks
joshuaellis added a commit that referenced this issue Jan 28, 2021
* [Feature] move stories to TS (#223) (#231)

* feat: setup TS for stories

* fix: eslint was ignore .storybook

* feat: add TS to storybook

* refactor: move stories to TS (WIP)

* refactor: move more stories to TS (WIP)

* refactor: src changes for TS move over

I think these were TS problems that were missed.

* fix: ContactShadows story

* refactor: move more stories to TS

* refactor: add MapControls.tsx after raising issues in three

Two errors linked to three.js:
mrdoob/three.js#21058
mrdoob/three.js#21059

* fix: TS errors for passing refs & children

* refactor: add more stories (WIP)

* fix: useContextBridge TS to accept array of children

fixed using DefinitelyTyped/DefinitelyTyped#44572 (comment) because microsoft/TypeScript#14729

* refactor: convert more stories to TS

also useGLTF does not need to declare it's type

* chore: update storybook

* fix: revert useTexture

accidentally committed a WIP change

* refactor: move stories to TS (WIP)

* refactor: revert useGLTF story

* Minor type fixes

* chore: update react-three-fiber to latest

Required for Types

* refactor: move stories to TS

* fix: shaderMaterial on init returns void, not null

* refactor: remove unnecessary type in useFBX

* refactor: type Environment stronger

there was issues with useAsset not understanding it's types....

* Fixes CSB CI (#230)

Co-authored-by: Gianmarco Simone <gianmarcosimone89@gmail.com>

Co-authored-by: Josh <37798644+joshuaellis@users.noreply.github.com>

* 2.2.16

* chore: add templates (#232)

* chore: add react-dom as optional peer depency

if you're using react-native you don't need it.

* docs: update readme

* fix: fix storybook imports that should have been done before

* BREAKING CHANGE: refactor internal to solve react-native imports

my bad.

Co-authored-by: Gianmarco <gianmarcosimone89@gmail.com>

* 2.3.0-beta.0

* fix: edit transformOutputPath to remove .ts from files

* 2.3.0-beta.1

* fix: useAnimations ref type & chore: add storybook entry for useAnimations

* Added THREE.Group to useAnimations types

* Small refactor

* Added useAnimations story

* Clean up storybook

* Fix null ref typing on useAnimations

* Forgot to change typing on useAnimations api

* chore: add storybook entry to README

Co-authored-by: Josh Ellis <joshua.ellis18@gmail.com>

* [fix] Blob is undefined when using SSR (#239)

* fix: #233 Blob is undefined in SSR

* refactor: it appears Billboard is fixed

Looks like Typescript does not suck.

* 2.3.0-beta.2

* fix: create polyfill for atob

* 2.3.0-beta.3

* chore: update readme in response to #240

* fix: #242 Frustum has incorrect spelling in softshadows (#243)

* fix: suggestion for fixing #242

* refactor: default should be other way round

* chore: update readme with correct spelling

* 2.3.0-beta.4

* feat: Add MeshoptDecoder support to useGLTF (#246)

* Add MeshoptDecoder support to useGLTF

* chore: move center

Co-authored-by: Josh Ellis <joshua.ellis18@gmail.com>

* 2.3.0-beta.5

* feat: #181 – adding points to line (#247)

* feat: re-create geometry on new points

* refactor: useMemo the geometry instead of useEffect

* 2.3.0-beta.6

* 3.0.0-beta.1

* 3.0.0-beta.2

* feat: update troika with new features (#255)

* 3.0.0-beta.3

* feat: add ability to add custom scene as prop (#256)

* feat: add ability to add custom scene as prop

* docs: update Environment with new feats

* 3.0.0-beta.4

* refactor: shuffle imports for beta

* 3.0.0-beta.5

* docs: clearly specify name of package & old has been deprecated

* BREAKING CHANGE: Upgrade Three.js to r125 (#262)

* chore: update Three.js dependency

* refactor: remove subdivision modifier

* fix: remove use of FaceNormalsHelper as it no longer exists in Three

* fix: update typings for THREE

* fix: update modifiers to work without THREE.Geometry

* docs: update README

* chore: add Three.js peer dependency

* fix: change install instructions to use yarn instead of npm (#265)

* repurpose reflector, make it capable to blur (#260)

* repurpose reflector, make it capable to blur

* refactor: move materials out to reduce noise & fix types

* refactor: merge beta

* fix: add child propType

* refactor: update reflector story & fix children type

* changes

* fix: shuffle types so reflectorProps come from what the material expects

* docs

Co-authored-by: Josh Ellis <joshua.ellis18@gmail.com>

* chore: add workflows to beta release

* BREAKING CHANGE force build

* 3.0.0-beta.6

Co-authored-by: Gianmarco Simone <gianmarcosimone89@gmail.com>
Co-authored-by: Tanner Hartwig <tanner.hartwig57@gmail.com>
Co-authored-by: Matt Rossman <rossman@gatech.edu>
Co-authored-by: Renaud Rohlinger <renaud.rohlinger@gmail.com>
Co-authored-by: Jure Triglav <juretriglav@gmail.com>
Co-authored-by: Jason Bixon <jbixon13@gmail.com>
Co-authored-by: Paul Henschel <drcmda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released on @beta released in beta for review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants