-
-
Notifications
You must be signed in to change notification settings - Fork 589
Attribute-based vector format refactor #1624
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
Conversation
This looks like a great start to making this all spreadsheet-based in the near future! Can't wait to have a spreadsheet view and the ability to tack on extra columns and read from those in nodes throughout the graph.
Those are my only three comments! None are merge blockers. I just need to do my QA testing now that I finished reading the code changes. Impressive work here. |
fn recurse(a0: DVec2, a1: DVec2, a2: DVec2, level: u8, desired_len: f64) -> (f64, f64) { | ||
let lower = a0.distance(a2); | ||
let upper = a0.distance(a1) + a1.distance(a2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it makes sense to use the squared distance here instead to reduce the number of square root computations, the algorithm should still converge, but the convergence could be faster, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean? (a+b)(a+b) ≠ a*a+b*b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm basically suggesting to replace distance
with distance_squared
and then only computing the square root at the end. Let c = ((dist₀₂)² + (dist₀₁)² + (dist₁₂)²) / 2. Then dist₀₂ <= √c <= (dist₀₁ + dist₁₂).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But to be clear, this is in no way a release blocker, more of a "I'm wondering if this could be a future performance improvement"
I tested for regressions from the nodes listed as changed within Here's a video showing this with the Repeat node: capture_2_.mp4Here's a list of all the nodes this happens with:
I also noticed the And there's also one node which seems to actually have been fixed from previously broken behavior:
I don't notice any other issues, so if you're able to fix the three nodes here, we should be good to merge! Thank you for all your hard work on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments are minor enough that they don't need to be merge blockers. Feel free to merge this anytime.
Closes #811