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

refactor: optimize waypoint drawing #2561

Merged
merged 9 commits into from
Oct 15, 2024
Merged

refactor: optimize waypoint drawing #2561

merged 9 commits into from
Oct 15, 2024

Conversation

MonstraG
Copy link
Contributor

@MonstraG MonstraG commented Oct 14, 2024

I've realized it would take ages merging these one by one. You can review each commit one by one, each is self contained.

  • refactor: optimize jquery calls in drawWaypointSegment:

First I did not like the performance of the drawWaypointSegment function and did the first thing that came to mind - avoid jquery selector duplications.

  • refactor: remove ruler-svg-bobbles:

Whist testing that, I realized that ruler-svg-bobbles never actually show up on the screen, so they could just be removed.

  • refactor: optimize draw loop by moving all dom operations out:

After doing that, I noticed that drawWaypointSegment is called in a loop and moved everything up, and replaced it with native getElementById which was much faster than jquery operations.

  • refactor: remove midlineLabels as unused:

While doing that, I noticed that midlineLabels was always false, so I removed it.

  • fix: remember labelX and labelY to avoid label jump on mouse up

Finally, I felt like I knew enough about how this works to fix another thing that has been bugging me: the label would jump up as soon as I release LMB because fadeaway redrawing did not remember the labelX and labelY args.

You can look at flamegraph for draw before my changes in #2560.

After all changes it looks like this:
image

@Azmoria
Copy link
Collaborator

Azmoria commented Oct 14, 2024

Bobbles are the end/mid points of the lines. They do show up on main and should be left in.

I also only see the jump up when dragging a token. This is due to it keeping it outside the token while dragging then moving it to the center spot on release. This is on purpose as we want the text off the token as we drag it and the text close to the vertex when we drag the token again. With this PR you currently get a very large gap when dragging a gargantuan creature.

For example this PR:

image

vs main:

image

Mostly the performance issue for the ruler is when dragging tokens with the console open - not the ruler function itself. Since people don't play with the console open I've ignored it. I agree it'd be nice if it was better but I think this has to do with the token dragging function more then the ruler function.

This is originally why it had to be specifically enabled when dragging tokens - although there's been improvements since then that mostly eliminated that issue and contained it to having the console open.

Happy to have some of these other changes if they improve performance while maintaining the other functionality above.

As a note in testing this PR I have noticed sometimes a multi segmented ruler (ruler tool + right click) fails to keep it's first segment positioned correctly but I don't think it's related to this PR.

@MonstraG
Copy link
Contributor Author

Thanks, I'm gonna revert the bubbles and labelX/labelY changes.

I drag and measure with tokens much more than with standalone ruler, so both performance and the jump seem to bug me more than you)

@Azmoria
Copy link
Collaborator

Azmoria commented Oct 14, 2024

Fair - I also have token drag measurement on all the time I just don't see performance issues on my end except with the dev tools open. Mind you I do play with a bunch of people who have higher end gaming PCs so it's pretty easy for us to miss those things.

As far as I can tell the dev tools don't like removing/adding the ruler dom elements repeatedly while dragging a token (the svg stuff since they don't update when just changing the values - and canvas is a non-option due to scaling blurring the text and was also less performant when we did use canvas).

I think for the jumping the only other option I can think of is putting the text in the center regardless of token but I think that looks worse then it jumping when you drop the token and pick it up again. This might just be preference.

Edit: Alternatively have it only jump up when the token is picked up again - so midpoints still have their label close by. Otherwise it stays below the token as it fades. I might like this option most tbh. I can also take a look at doing this depending on if you want to pick it up.

@MonstraG
Copy link
Contributor Author

MonstraG commented Oct 15, 2024

Here's how these bobbles look like on my screen (windows, chrome, 1440p, 1x scale):
image

and my main browser, firefox:
image

That's why I couldn't find them.

I pushed the reverts V but I think we have to make them more visible lol

Yea, firefox just refuses to show those circles.

@MonstraG
Copy link
Contributor Author

MonstraG commented Oct 15, 2024

Figured out why they did not show up on firefox.

Firefox thinks that calc(2 / 1.5) is not a valid value, calc(2px / 1.5) is. https://developer.mozilla.org/en-US/docs/Web/CSS/calc will helpfully show you when the value is not valid.

Well, I just tried fixing it and it half-worked, the lines would be drawn correctly only if I go into devtools and uncheck/recheck the rules. That's hella weird. I'm just not gonna touch that, firefox will have to cry in a corner. F

@Azmoria
Copy link
Collaborator

Azmoria commented Oct 15, 2024

I hadn't noticed that in firefox - I merged a PR into main that should fix that. There was a couple other places where the units were missing in the css like you mentioned. It definitely should always have units on one side of the calculation - was just missed in this case.

image

I applied the same changes manually to this PR and it seems to be fine. I'll do some more testing and merge in a bit if you think it's ready to merge.

Looks like this in firefox for me now (no dev tool editing).

image

@MonstraG MonstraG changed the title fix: remember labelX and labelY to avoid label jump on mouse up refactor: optimize waypoint drawing Oct 15, 2024
@MonstraG
Copy link
Contributor Author

Merged main in, checked it out in firefox, bobbles show up now.

lgtm)

@Azmoria Azmoria merged commit d9e35c9 into cyruzzo:main Oct 15, 2024
1 check passed
@MonstraG MonstraG deleted the patch-7 branch October 21, 2024 18:58
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.

2 participants