Skip to content

feat: wrapEffect disposal & root context #179

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

Merged
merged 10 commits into from
Apr 26, 2023

Conversation

CodyJasonBennett
Copy link
Member

@CodyJasonBennett CodyJasonBennett commented Feb 26, 2023

Fixes #25
Fixes #42
Fixes #132
Fixes #157
Fixes #173

Creates a private element identifier via extend rather than the use of hooks/primitive since the latter does not dispose nor is managed by React. Consequently, wrapped effects can use the full featureset of React and R3F, including args and attach. I've defined getters/setters for any prop aliases and camera defaults from host context for effects that need them.

@CodyJasonBennett CodyJasonBennett marked this pull request as draft February 26, 2023 18:32
@talentlessguy
Copy link
Member

I'm working on updatin rpp (#180) by the way and I'll try to sync it up with your PR @CodyJasonBennett

@CodyJasonBennett
Copy link
Member Author

I can rebase/redo this one, too. I think there's a less invasive way to do this that isn't breaking.

@CodyJasonBennett CodyJasonBennett marked this pull request as ready for review February 28, 2023 17:53
@talentlessguy
Copy link
Member

Copy-pasted code to the demo (I used it for my PR also) - works flawlessly: https://codesandbox.io/s/ancient-sky-ldkxwk?file=/src/App.tsx

@talentlessguy talentlessguy requested a review from drcmda March 1, 2023 10:56
@talentlessguy
Copy link
Member

@CodyJasonBennett is this ready for merging?

@CodyJasonBennett
Copy link
Member Author

I'll need to create sandboxes for each of the linked issues to confirm they're good to close, then yeah.

@chasedavis
Copy link
Contributor

Is there any hope of getting this one over the line @talentlessguy @CodyJasonBennett ? This looks like a major upgrade to rpp flexibility!

@talentlessguy
Copy link
Member

@chasedavis sorry for the waiting, the sandboxes aren't ready yet... but if you'd like this to be merged quicker, help is appreciated! we need to create a code sandbox (or multiple) to test if all the effects are working correctly

I'll create a checklist

@talentlessguy
Copy link
Member

  • Noise
  • Glitch
  • Pixelation
  • BrightnessContrast
  • DotScreen
  • Scanline
  • ToneMapping
  • SSAO
  • GodRays
  • Shockwave
  • Bloom
  • Outline
  • Sepia
  • HueSaturation
  • ChromaticAbberation
  • ColorAverage
  • Grid
  • SMAA
  • SSR
  • Tiltshift

@drcmda
Copy link
Member

drcmda commented Apr 25, 2023

@talentlessguy do we need all of them before we can merge?

@CodyJasonBennett
Copy link
Member Author

CodyJasonBennett commented Apr 25, 2023

@drcmda I refactored this to be less invasive. This is good to merge.

@CodyJasonBennett CodyJasonBennett changed the title fix: wrapEffect disposal & root context feat: wrapEffect disposal & root context Apr 26, 2023
@talentlessguy
Copy link
Member

talentlessguy commented Apr 26, 2023

ok then merging, if something breaks we can reopen issues

@talentlessguy talentlessguy merged commit 5f3a30e into pmndrs:master Apr 26, 2023
@CodyJasonBennett CodyJasonBennett deleted the fix/wrap-extend-ctx branch April 26, 2023 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants