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

Submit tile marker metronome #7214

Merged
merged 4 commits into from
Jan 25, 2025
Merged

Conversation

RenaudBernon
Copy link
Contributor

Resubmit of #6998

This plugin combines Tile markers with the visual metronome.
Marked tiles change color every X ticks.

In the last MR the comment was made that since I was only keeping track of ticks for loaded tiles, this would effectively sync the metronome with some boss encounters, making mechanics that happen after X ticks trivial.
To combat this I am now keeping a global tick counter. This hopefully alleviates that concern.

@runelite-github-app
Copy link

runelite-github-app bot commented Jan 8, 2025

@LlemonDuck
Copy link
Contributor

These aren't blockers but you should want to change them anyway:

This is going to cause parity issues where some groups don't roll over when they should if gcd(100, tickCount) is 1. If you were concerned about the variable becoming too large, it just won't. You'd need like 40 years of gametime to overflow the variable:

if (currentTick > 100) {
    currentTick = 1;
}

Your currentTick++ should be in onGameTick not tickGroup, otherwise you're ticking each group each tick times the number of groups, not by the number of ticks that have happened.

Marking currentTick as transient does nothing,

https://github.com/RenaudBernon/tile-marker-metronome/blob/aa37949e9a22577b5f8872048240046674ffc178/src/main/java/com/tilemarkermetronome/TileMarkerMetronomePlugin.java#L121-L128


To avoid your menu entry from looking identical to the core options, you should change your "Mark" and "Unmark" to something like "Tick-Mark" or literally anything else that isn't "Mark".

https://github.com/RenaudBernon/tile-marker-metronome/blob/aa37949e9a22577b5f8872048240046674ffc178/src/main/java/com/tilemarkermetronome/TileMarkerMetronomePlugin.java#L164-L166


This doesn't need to be atomic, there's no multithreading going on:

https://github.com/RenaudBernon/tile-marker-metronome/blob/aa37949e9a22577b5f8872048240046674ffc178/src/main/java/com/tilemarkermetronome/TileMarkerMetronomeGroup.java#L64

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 10, 2025
@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 10, 2025
@RenaudBernon
Copy link
Contributor Author

Thanks for the feedback, Ive made changes according to the comments.

Except for

This doesn't need to be atomic, there's no multithreading going on:

https://github.com/RenaudBernon/tile-marker-metronome/blob/aa37949e9a22577b5f8872048240046674ffc178/src/main/java/com/tilemarkermetronome/TileMarkerMetronomeGroup.java#L64+

I'm using an AtomicInteger because variables used in streams need to be effectively final. I wouldn't be able to increment a normal int.

@LlemonDuck
Copy link
Contributor

Don't do Swing work on the client thread, use SwingUtilities#invokeLater for e.g. https://github.com/RenaudBernon/tile-marker-metronome/blob/5f01834ffce27271d91c5ceaf80edde8da4f6d7b/src/main/java/com/tilemarkermetronome/TileMarkerMetronomePlugin.java#L140


Downgrade this and other frequent log.info calls to log.debug instead https://github.com/RenaudBernon/tile-marker-metronome/blob/5f01834ffce27271d91c5ceaf80edde8da4f6d7b/src/main/java/com/tilemarkermetronome/TileMarkerMetronomeGroup.java#L73


for (int i = 0; i < pointIndex; i++) {
  currentTileColor++;
  currentTileColor %= colors.size();
}

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 11, 2025
@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 20, 2025
@LlemonDuck LlemonDuck merged commit 7810a37 into runelite:master Jan 25, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants