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: native threejs effects #121

Merged
merged 30 commits into from
Sep 26, 2024
Merged

Conversation

Tinoooo
Copy link
Contributor

@Tinoooo Tinoooo commented Sep 8, 2024

No description provided.

@Tinoooo Tinoooo requested a review from alvarosabu September 8, 2024 11:17
@Tinoooo Tinoooo self-assigned this Sep 8, 2024
@Tinoooo Tinoooo linked an issue Sep 8, 2024 that may be closed by this pull request
@Tinoooo Tinoooo changed the title feat; native threejs effects feat: native threejs effects Sep 8, 2024
@Tinoooo Tinoooo marked this pull request as ready for review September 8, 2024 11:33
@Tinoooo Tinoooo marked this pull request as draft September 8, 2024 11:37
@Tinoooo Tinoooo marked this pull request as ready for review September 8, 2024 12:08
Copy link
Member

@alvarosabu alvarosabu left a comment

Choose a reason for hiding this comment

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

Hey @Tinoooo amazing job with this, I think it was a great idea to separate both postprocessing and native ones.

Only 3 merge-blocking topics:

Let me know if you need help with any of those

<script lang="ts">
import { useLoop, useTresContext } from '@tresjs/core'
import { useDevicePixelRatio } from '@vueuse/core'
import { EffectComposer as EffectComposerThreejs } from 'three/examples/jsm/postprocessing/EffectComposer.js'
Copy link
Member

Choose a reason for hiding this comment

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

Hi @Tinoooo change all the three/examples/jsm imports to three-stdlib which is a stand-alone version of threejs/examples/jsm written in Typescript & built for ESM & CJS. This way we ensure proper tree-shaking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to use three-stdlib, but their EffectComposer work differently. It does not have a dispose method, which is important for memory stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this changed: pmndrs/three-stdlib#378
But I was suggested by the three-stdlib dev not to use their package if we only want to support ESM. three-stdlib also does not have the OutputPass. I suggest sticking to /examples/jsm.

@@ -0,0 +1,30 @@
<script lang="ts">
import { GlitchPass } from 'three/examples/jsm/postprocessing/GlitchPass.js'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { GlitchPass } from 'three/examples/jsm/postprocessing/GlitchPass.js'
import { GlitchPass } from 'three-stdlib'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please see comment above

import type { Blending } from 'three/src/constants.js'
import { useEffect } from './composables/useEffect'

export const Dot = 1
Copy link
Member

Choose a reason for hiding this comment

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

Maybe enum? (Non-blocking)

export enum HalftoneShape {
  Dot = 1,
  Ellipse = 2,
  Line = 3,
  Square = 4
}

Copy link
Contributor Author

@Tinoooo Tinoooo Sep 12, 2024

Choose a reason for hiding this comment

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

Yes! Much better 👍. I added that.

@@ -0,0 +1,8 @@
<script lang="ts" setup>
import { OutputPass } from 'three/examples/jsm/postprocessing/OutputPass.js'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { OutputPass } from 'three/examples/jsm/postprocessing/OutputPass.js'
import { OutputPass } from 'three-stdlib'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please see comment above

<script lang="ts" setup>
const props = defineProps<GlitchProps>()

const { pass } = useEffect(() => new GlitchPass(props.dtSize))
Copy link
Member

Choose a reason for hiding this comment

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

@Tinoooo does this pass have a callback every time they trigger the glitch? Since it could be considered as an animation we should invalidate the frames otherwise it will only render the effect once while using on-demand (see image below)

If the effect has no callback, maybe we can fake it by only adding an onBeforeRender that invalidates once this is mounted

Screenshot 2024-09-11 at 10 16 17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it for glitch and noise. For some reason the outline effect does not work with on-demand rendering. Also on main. I added a bug issue for that (#125).

@alvarosabu alvarosabu added feature p3-significant High-priority enhancement (priority) labels Sep 11, 2024
@Tinoooo
Copy link
Contributor Author

Tinoooo commented Sep 12, 2024

Hey @Tinoooo amazing job with this, I think it was a great idea to separate both postprocessing and native ones.

Only 3 merge-blocking topics:

Let me know if you need help with any of those

I added the prop change watcher. Let's handle the rest in their respective PR threads.

@Tinoooo Tinoooo requested a review from alvarosabu September 22, 2024 20:10
Copy link
Member

@alvarosabu alvarosabu left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to work on this and address all the comments. Amazing work as always @Tinoooo

@alvarosabu alvarosabu merged commit cd24a57 into main Sep 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature p3-significant High-priority enhancement (priority)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Native three.js effects
2 participants