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

Rewrite Graph component calculations #493

Merged
merged 3 commits into from
Dec 28, 2021
Merged

Rewrite Graph component calculations #493

merged 3 commits into from
Dec 28, 2021

Conversation

kadiwa4
Copy link
Contributor

@kadiwa4 kadiwa4 commented Dec 26, 2021

The goal of this PR is to make src/component/graph.rs easier to understand. To do that, I restructured the functions, which makes the diff very hard to read. Hopefully the new code structure can compensate for that. The functions are mostly executed in the order they're defined in. I didn't make any changes that break the public API, the graph looks almost exactly the same. I tested this with livesplit-one-desktop and the browser LSO version.

Some examples of things that are bad about the master version of this file:

  • The result of calculate_left_side_coordinates is never used
  • "Graph" is used to refer to both the graph and the entire diagram in comments
  • In a lot of calculations it is hard to tell why some of the variables are used or they have unclear names
  • WIDTH and HEIGHT have arbitrary values (180 and 120) and the graph is later transformed into a 0..1 square

@kadiwa4
Copy link
Contributor Author

kadiwa4 commented Dec 26, 2021

The reason the tests are failing:
Previously, the horizontal grid lines in the upper half were 1/120 higher and I think the x-axis was rendered as two lines (whereas it's only rendered once now).

I'll replace the expected hash (with SXfHSWVpRlc=).

Copy link
Member

@wooferzfg wooferzfg left a comment

Choose a reason for hiding this comment

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

We might still want the x-axis to be two lines so that it's thicker than the rest of the grid lines and so that it's included in both the positive and negative regions? It would be helpful if you could post screenshots of both the old and new graph for both LSO Desktop and LSO on the web.

src/component/graph.rs Show resolved Hide resolved
@kadiwa4
Copy link
Contributor Author

kadiwa4 commented Dec 26, 2021

left half = old version
right half = my new version
https://cdn.discordapp.com/attachments/667816507614691368/924743455362670612/comparison_images.zip
it's symmetric now but a bit less visible

@CryZe CryZe added code quality Affects the quality of the code. enhancement An improvement for livesplit-core. rendering The issue or pull request is affecting the rendering. labels Dec 26, 2021
@CryZe CryZe added this to the v0.13 milestone Dec 28, 2021
@CryZe CryZe merged commit 96e47a3 into LiveSplit:master Dec 28, 2021
@kadiwa4 kadiwa4 deleted the graph branch December 28, 2021 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Affects the quality of the code. enhancement An improvement for livesplit-core. rendering The issue or pull request is affecting the rendering.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants