Generate outlines 2 #75
Conversation
…nto generate-outlines-2
There was a problem hiding this comment.
Awesome work @KeyboardSounds 👏 always been impressed by your outline code from the i3s to 3dtiles! Most of my comments are just questions for clarification/attempt to help improve the performance (a tiny bit) 😬 but honestly it already looks really good! 👍 I'm glad I was able to thoroughly review (and learn from) it too so I have a bit of a headstart with my work on improving cesium's outlines :)
| !ModelOutlineLoader.shouldGenerateOutlines(model) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| if (ModelOutlineLoader.shouldGenerateOutlines(model)) { |
There was a problem hiding this comment.
I don't know how expensive it is to run the shouldGenerateOutlines function, but I suspect instead of running it twice it might be quicker to just store the boolean as a variable and then use that in both if statements?
Or is JS smart enough not to run that function twice? (PS genuinely don't know and would like to know if JS is that smart!)
There was a problem hiding this comment.
The short answer is yes, most JS engines will cache the output of most functions if they get called multiple times with the same parameters. It doesn't always do this though, optimising compilers generally only bother if it's worth the effort— like if a function is being called many, many times.
The exact circumstances under which a particular bit of code will be optimised or not are hard to pin down, because the rules are complicated and subtle. As a general rule of thumb, I like to write code that is easy for compilers to optimise, as if it's not going to get optimised. So yes, even though the compiler would probably cache the result of this function if it got called a bunch of times, I should cache the result, you're absolutely right.
I couldn't find a good article on this exact scenario but https://ponyfoo.com/articles/an-introduction-to-speculative-optimization-in-v8 and https://mathiasbynens.be/notes/shapes-ics are some long reads that explain some of the general principles at play.
There was a problem hiding this comment.
Oh EXCEPT I just went to implement caching for it and it's kind of a pain because ModelOutlineLoader is a static class and doesn't store state per model. so never mind
There was a problem hiding this comment.
Ah right, what if it was just a var shouldGenerateOutlinesForModel = shouldGenerateOutlines(model)? 🤔 and use that boolean instead of calling the function twice?
Though now that you added the break statement it's probably not such a bad thing to leave this out since in theory the shouldGenerateOutlines function shouldn't take as long to run :)
There was a problem hiding this comment.
This looks really good and clear now, thank you @KeyboardSounds!
| !ModelOutlineLoader.shouldGenerateOutlines(model) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| if (ModelOutlineLoader.shouldGenerateOutlines(model)) { |
There was a problem hiding this comment.
Ah right, what if it was just a var shouldGenerateOutlinesForModel = shouldGenerateOutlines(model)? 🤔 and use that boolean instead of calling the function twice?
Though now that you added the break statement it's probably not such a bad thing to leave this out since in theory the shouldGenerateOutlines function shouldn't take as long to run :)
…ause it was causing minify errors
…isulaizer, fixed outline generation bugs
No description provided.