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: sampler and useSampler #286

Merged
merged 5 commits into from
Nov 29, 2023
Merged

feat: sampler and useSampler #286

merged 5 commits into from
Nov 29, 2023

Conversation

JaimeTorrealba
Copy link
Member

closes #156

Copy link

stackblitz bot commented Nov 17, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

netlify bot commented Nov 17, 2023

Deploy Preview for cientos-tresjs ready!

Name Link
🔨 Latest commit 80383b1
🔍 Latest deploy log https://app.netlify.com/sites/cientos-tresjs/deploys/6565bdaa4ef273000865797b
😎 Deploy Preview https://deploy-preview-286--cientos-tresjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JaimeTorrealba JaimeTorrealba marked this pull request as draft November 17, 2023 14:28
@JaimeTorrealba JaimeTorrealba marked this pull request as ready for review November 20, 2023 17:50
@JaimeTorrealba JaimeTorrealba self-assigned this Nov 25, 2023
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.

Pretty neat implementation, just consider changing the naming convention to SurfaceSampler to be explicit about the functionality and some minor stuff

Great job buddy


export type TransformFn = (payload: TransformPayload, i: number) => void

export const useSampler = (
Copy link
Member

Choose a reason for hiding this comment

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

Hey @JaimeTorrealba small feedback for the naming.

useSampler sounds too generic, I would have called this useSurfaceSampler because it is more clear and verbose on the functionality that covers + the original class from threejs is MeshSurfaceSampler

weight?: string,
transform?: TransformFn,
) => {
const buffer = ref(new InterleavedBuffer(new Float32Array(count * 16), 16))
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
const buffer = ref(new InterleavedBuffer(new Float32Array(count * 16), 16))
const arr = new Float32Array(count * 16)
const buffer = ref(new InterleavedBuffer(arr, 16))

More readable :)

InterleavedBuffer,
} from 'three'
import type { Mesh, InstancedMesh } from 'three'
import { MeshSurfaceSampler } from 'three/examples/jsm/math/MeshSurfaceSampler'
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 { MeshSurfaceSampler } from 'three/examples/jsm/math/MeshSurfaceSampler'
import { MeshSurfaceSampler } from 'three-stdlib'

Avoid exporting these extra classes from three since they are not three-shakeable and not ESM compatible, use three-stdlib instead

@alvarosabu alvarosabu added the v4 label Nov 28, 2023
@@ -0,0 +1,31 @@
<!-- eslint-disable max-len -->
Copy link
Member

Choose a reason for hiding this comment

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

@JaimeTorrealba avoid as much as possible to add disable rules on top of the file, use the above this line option

@alvarosabu alvarosabu merged commit 826833c into main Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sampler
2 participants