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

P8 NGE linear and radial cloner blocks #14172

Merged
merged 16 commits into from
Oct 10, 2023

Conversation

Pryme8
Copy link
Contributor

@Pryme8 Pryme8 commented Aug 16, 2023

https://forum.babylonjs.com/t/nge-linear-cloner/43369

Just a simple linear cloner.

Only supports uv0 right now.

Probably should add color support as well.

@bjsplat
Copy link
Collaborator

bjsplat commented Aug 16, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@deltakosh
Copy link
Contributor

deltakosh commented Aug 16, 2023

Why not cloning with vertexData.clone and then applying the correct transformations on each clone?

something like that:

const geometry = (value as VertexData).clone();
                    geometry.transform(matrix);
                    return geometry;

where matrix is the overall transformation build from the parameters you have on the block

@Pryme8
Copy link
Contributor Author

Pryme8 commented Aug 16, 2023

Why not cloning with vertexData.clone and then applying the correct transformations on each clone?

something like that:

const geometry = (value as VertexData).clone();
                    geometry.transform(matrix);
                    return geometry;

where matrix is the overall transformation build from the parameters you have on the block

I had no clue you could do this, will have to refactor it to use that.

@bjsplat
Copy link
Collaborator

bjsplat commented Aug 17, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Aug 17, 2023

@Pryme8 Pryme8 changed the title P8 NGE linear cloner block P8 NGE linear and radial cloner blocks Aug 18, 2023
@deltakosh
Copy link
Contributor

Ok I feel like we need a general clone as well. Something that takes a geometry, a count, a rotation/scaling/translation and simply instantiate the geometry n times. The inputs (geometry and transformation can be static or dynamic if for instance we create a new CloneID value that will give you the current index (from 0 to count). I will add this node after we have merged this PR :)

@deltakosh
Copy link
Contributor

@Pryme8 just for naming my block will be CloneBlock so maybe we can name yours LinearCloneBlock and RadialCloneBlock?

@deltakosh
Copy link
Contributor

@Pryme8 any update buddy?

@Pryme8
Copy link
Contributor Author

Pryme8 commented Sep 19, 2023

I totally spaced out about these, my bad. Ill try go resolve this stuff today or tomorrow!

Copy link
Contributor

@deltakosh deltakosh left a comment

Choose a reason for hiding this comment

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

Everything is fine. We simply need to look at the way InstantiateBlock operates and do the same.

@Pryme8
Copy link
Contributor Author

Pryme8 commented Sep 22, 2023

@deltakosh should be good to go now!

@deltakosh
Copy link
Contributor

Thanks!!

Copy link
Contributor

@deltakosh deltakosh left a comment

Choose a reason for hiding this comment

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

Ok sorry to be a pain :)

Here is my idea: as these 2 classes are almost a clone of InstantiateBlock, what if we simply inherits from it and replace the '_buildBlock' function?

Also @PatrickRyanMS, regarding naming, maybe we should name then like LinearInstantiateBlock and RadialInstantiateBlock?

@PatrickRyanMS
Copy link
Member

Ok sorry to be a pain :)

Here is my idea: as these 2 classes are almost a clone of InstantiateBlock, what if we simply inherits from it and replace the '_buildBlock' function?

Also @PatrickRyanMS, regarding naming, maybe we should name then like LinearInstantiateBlock and RadialInstantiateBlock?

I would suggest we follow the same format we've already established with the word "instantiate" first so that all nodes in the instances group begin with "instantiate." So instantiateLinear and instantiateRadial. I would also suggest to order them in the node list right beneath instantiate in alpha order:

  • instantiate
  • instantiateLinear
  • instantiateRadial
  • instantiateOnFaces
  • instantiateOnVertices
  • instantiateOnVolume

@Pryme8
Copy link
Contributor Author

Pryme8 commented Sep 26, 2023

Ok, if I get a chance tonight ill fix that up. If not then it will have to be tomorrow while I'm off work!

@Pryme8
Copy link
Contributor Author

Pryme8 commented Oct 4, 2023

Finally was able to get to this, sorry about the delay.
@deltakosh

@Pryme8
Copy link
Contributor Author

Pryme8 commented Oct 4, 2023

its yelling at me about the linting but when I do npm run lint it only brings up warnings about js docs?

@sebavan
Copy link
Member

sebavan commented Oct 4, 2023

cc @RaananW for the lint/format issue.

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

this is the formatting issue

…ListComponent.tsx

Co-authored-by: Raanan Weber <raananw+github@gmail.com>
@Pryme8
Copy link
Contributor Author

Pryme8 commented Oct 5, 2023

Is it time to roll the beautiful bean footage?

@Pryme8 Pryme8 requested a review from deltakosh October 8, 2023 21:11
Copy link
Contributor

@deltakosh deltakosh left a comment

Choose a reason for hiding this comment

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

Almost there ;)

@deltakosh deltakosh merged commit b6fd48e into BabylonJS:master Oct 10, 2023
10 checks passed
@deltakosh
Copy link
Contributor

Thanks a ton @Pryme8 !!!

@Pryme8 Pryme8 deleted the p8-nge-linear-cloner-block branch October 10, 2023 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants