Conversation
kmorel
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing. This fell in my email.
Overall this looks good. I didn't do a deep dive, but the code looks reasonable.
Before this can be merged, the CI has to pass. The first problem is that it is failing the formatting test. You can get a patch file from GitHub to do that for you. (See CONTRIBUTING for details). It looks like the formatting test is holding up other tests, so we will have to see if those pass.
I would also prefer that the commits get squashed before being merged.
| const viskores::Float64 dx = this->GlobalBounds.X.Max - this->GlobalBounds.X.Min; | ||
| const viskores::Float64 dy = this->GlobalBounds.Y.Max - this->GlobalBounds.Y.Min; | ||
| const viskores::Float64 dz = this->GlobalBounds.Z.Max - this->GlobalBounds.Z.Min; | ||
| const viskores::Float64 ex = std::max<viskores::Float64>(dx, 0.f); | ||
| const viskores::Float64 ey = std::max<viskores::Float64>(dy, 0.f); | ||
| const viskores::Float64 ez = std::max<viskores::Float64>(dz, 0.f); |
There was a problem hiding this comment.
This is correct but could be more easily obtained with the following.
| const viskores::Float64 dx = this->GlobalBounds.X.Max - this->GlobalBounds.X.Min; | |
| const viskores::Float64 dy = this->GlobalBounds.Y.Max - this->GlobalBounds.Y.Min; | |
| const viskores::Float64 dz = this->GlobalBounds.Z.Max - this->GlobalBounds.Z.Min; | |
| const viskores::Float64 ex = std::max<viskores::Float64>(dx, 0.f); | |
| const viskores::Float64 ey = std::max<viskores::Float64>(dy, 0.f); | |
| const viskores::Float64 ez = std::max<viskores::Float64>(dz, 0.f); | |
| const viskores::Float64 ex = this->GlobalBounds.X.Length(); | |
| const viskores::Float64 ey = this->GlobalBounds.Y.Length(); | |
| const viskores::Float64 ez = this->GlobalBounds.Z.Length(); |
|
Oh, this PR could also use a changelog. It's how we collect features for release notes. Not enough changelogs can delay a release. |
|
Thanks. I didn't realize I asked for a review. I wanted to revisit a few things before. But will make the changes you suggest. |
No worries. GitHub sometimes automatically adds a recommended reviewer when you create a PR. I'm glad I was not delaying you. If you change several things and need a re-review, just request a new review and drop me a note. |
Worklet-ize the flow code.