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

Reduce calculation in strategy functions #753

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rwlee
Copy link

@rwlee rwlee commented Jan 19, 2023

A few optimizations in the strategy logic and lots of formatting from clang format

@rwlee rwlee requested a review from a team as a code owner January 19, 2023 21:59
@srimanachanta
Copy link
Member

srimanachanta commented Jan 20, 2023

your clang format seems to be buggy, the original documentation formatting was correct. Use gradle spotlessApply to fix it

@mcm001 mcm001 added this to the 2024 Kickkoff milestone Jan 27, 2023
Comment on lines -488 to +489
if (alternateTransformDelta < smallestHeightDifference) {
if (alternateTransformDelta < smallestHeightDifference
&& alternateTransformDelta < bestTransformDelta) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So the point of this function is to find a transform such that the Z difference is minimized right? I think we can better represent this intent with something similar to (but not necessarily a) list comprehension? Probably out of scope, and I don't think anyone uses this function anyways.

@mcm001 mcm001 removed this from the 2024 Kickkoff milestone Oct 15, 2023
@mcm001
Copy link
Contributor

mcm001 commented Oct 15, 2023

Waiting on code update so it builds

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.

3 participants