Generate skirt vertices and triangles - #12
Conversation
| } | ||
|
|
||
| getMeshWithSkirts(maxError = 0) { | ||
| const {gridSize: size, indices} = this.martini; |
There was a problem hiding this comment.
I agree in principle with reducing function bloat, and performance is very important to my use case too. I also believe it's beneficial to remove duplicate complex code and adding skits as an option to getMesh, if possible. Would you be open to combining the functions into one, together?
If you'd prefer to keep it this way, that is also fine. Are you looking for a 3rd party review? I can pull this down and test it out.
There was a problem hiding this comment.
The problem lies in getMesh being so concise and elegant :)
When I started adding if (skirt) blocks to handle the custom code I felt like I was making a mess. Once I started changing the vertices and triangle arrays as well as the return object (and more than doubling the length of the original function) I figured it might be neater to separate - though harder to maintain because of the code duplication of the core logic. I also thought a new user would have less trouble understanding getMesh by itself than getMesh with the skirt flag. @mourner any preference?
I'm guessing that the performance loss of the if (skirt) statement inside the recursive countElements function would be negligible but would have to bench at scale to be sure.
p.s. I also don't know if the way I'm generating the skirts is sound or if there's shortcuts etc that might make all of this moot
|
@chrisgervang any news on this ? would love to have skirts in deck.gl |
|
Maybe a repo owner can pull this in, since it seems all is ok? This is a pretty useful addition to the library. |
|
Agreed, seems like a very nice addition so considering a merge would be much appreciated! |
5 participants
Added a getMeshWithSkirts function to generate skirts while populating vertices and triangles arrays.
Could modify getMesh with a skirts flag but I didn't want to affect any non-skirts retrieval performance or bloat the function