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: add subdivide support #37

Closed
wants to merge 4 commits into from
Closed

feat: add subdivide support #37

wants to merge 4 commits into from

Conversation

u-u-z
Copy link

@u-u-z u-u-z commented Sep 3, 2023

cuz our package always used in the front-end part (website),
maybe show finer details through a small size stl model (optimization friendly) is needed

For example:
for my side, my website: kig.land use our packagereact-stl-viewer at index page to show models, cuz the model only need under 300kb

so I add https://github.com/stevinz/three-subdivide in our package, and add params subdivide (<StlViewer subdivide={2} />)

But I'm not sure it's a legitimate need,
so that I create a PR, if it needed I will add more (about documention).

@u-u-z u-u-z marked this pull request as draft September 3, 2023 14:47
@u-u-z u-u-z marked this pull request as ready for review September 3, 2023 14:48
@gabotechs
Copy link
Owner

Thanks for contributing!

I have a couple of questions regarding this PR:

  • I still don't fully understand the purpose of mesh subdivisions, could you elaborate a bit more on how is this beneficial to this package?
  • I have played a bit with the storybook example setting subdivisions={10} and it seems that the UI completely freezes and becomes unresponsive. What range of values are expected to be provided?

@u-u-z
Copy link
Author

u-u-z commented Sep 4, 2023

Thanks for contributing!

I have a couple of questions regarding this PR:

  • I still don't fully understand the purpose of mesh subdivisions, could you elaborate a bit more on how is this beneficial to this package?
  • I have played a bit with the storybook example setting subdivisions={10} and it seems that the UI completely freezes and becomes unresponsive. What range of values are expected to be provided?

Thanks for ur questions.

purpose & beneficial of subdivisions

The main purpose of subdivision is to increase the number of polygonal faces in a 3D model, it will result in smoother and more detailed rendering.

  • why we need this in front-end environment? : Due to resource constraints (protocol, traffic), sending mesh-rich models from the server in the Web will incur greater traffic overhead. so that, sending a "low poly" (size small) mesh and then smoothing on the client side will help with traffic optimization, it will bring more possibilities for front-end application development. don't worry about some detailed (sleek) models taking more traffic
  • beneficial for this package: cuz this package will work in web page (frontend) side, optimizing traffic (reducing the traffic occupied by requests) is a requirement in front-end engineering. Of course this needs to be balanced with performance optimizations that perform display (I'll be judging the parameters, it's normal not to use modifiers when users don't use the "subdivisions" parameter (next commit will fix that)

range of values

I'm sorry, my fault, cuz prepared not enough. the value subdivide between 1 and 3 will get good results, the 10 is really too big

ref from https://stevinz.github.io/three-subdivide/LoopSubdivision.html

My experiment results: 200kb stl (an animation mask face model) when set to 3
already has some lag on my iPhone 11 (no limit on max faces number), I will add more descriptions for params from https://github.com/stevinz/three-subdivide#usag in docs

@gabotechs
Copy link
Owner

Thanks for the detailed explanation!

I think this is a nice feature, lets do it, let me get back to you with a quick review later today

@u-u-z u-u-z changed the title feat: add subdivide support [WIP] feat: add subdivide support Sep 4, 2023
@u-u-z
Copy link
Author

u-u-z commented Sep 4, 2023

tested subdivideProps by manual: adjust maxTriangles to limit mesh faces numbers
it works

and docs added in 7e51754

a.mp4

@u-u-z u-u-z changed the title [WIP] feat: add subdivide support feat: add subdivide support Sep 4, 2023
Copy link
Owner

@gabotechs gabotechs left a comment

Choose a reason for hiding this comment

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

Looking good! I would suggest a couple of API changes for gathering all the subdivide related parameters into the same object

see first discussion below

src/StlViewer/SceneSetup.tsx Show resolved Hide resolved
src/StlViewer/SceneSetup.tsx Show resolved Hide resolved
src/StlViewer/SceneSetup.tsx Show resolved Hide resolved
@gabotechs
Copy link
Owner

I have some serious concerns about the performance of the three-subdivide library. I am doing some benchmarks for this STL: https://storage.googleapis.com/ucloud-v3/2272dfa00d58a59dae26a399.stl [748 Kb]

The results I am getting in the storybook example for that STL (748 Kb) in a Mac M1 PRO are:

1 Subdivisions - elapsed LoopSubdivision.modify call time: 686  ms
2 Subdivisions - elapsed LoopSubdivision.modify call time: 2919  ms
3 Subdivisions - elapsed LoopSubdivision.modify call time: 13399  ms
4 Subdivisions - elapsed LoopSubdivision.modify call time: 99841  ms
5 Subdivisions - elapsed LoopSubdivision.modify call time: indefinite freeze...

The result are measured by just wrapping the LoopSubdivision.modify with a time measurement:

const start = new Date().getTime()
const result = LoopSubdivision.modify(geometry, subdivide, subdivideProps)
console.log(`${subdivide} Subdivisions - elapsed LoopSubdivision.modify call time:`, new Date().getTime() - start, ' ms')

I would say that the performance does not look very acceptable I am afraid...

@gabotechs
Copy link
Owner

As the benchmarks are not giving very good results, it is possible that this change is not suitable to be merged, but I don't want to leave you empty handed, so how about this #38?

@u-u-z
Copy link
Author

u-u-z commented Sep 5, 2023

As the benchmarks are not giving very good results, it is possible that this change is not suitable to be merged, but I don't want to leave you empty handed, so how about this #38?

wow, amazing, thats awesome, thats great for flexibility 👏👏 Yeah ur right, benchmarks not good, I'm sorry I forgot it 😉 I should focus on "flexibility" not one things ~

@gabotechs
Copy link
Owner

Closing in favor of #38, which was merged and is available under release 2.3.0

@gabotechs gabotechs closed this Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants