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

Add unbreakable carpet highlighter #1034

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

Conversation

Emirlol
Copy link
Collaborator

@Emirlol Emirlol commented Oct 27, 2024

Renders a colored box around nearby carpets within mithril ore veins in dwarven mines.
image
It can be toggled on and off, and the color is configurable.
image
Uses up about 0.30% of tick time for scanning & rendering while in dwarven mines. Didn't test outside but it should be much less, probably not even worth mentioning as it's just a boolean if check on each tick.
image

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Oct 27, 2024
Copy link
Contributor

@olim88 olim88 left a comment

Choose a reason for hiding this comment

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

seems like good code to me.

@LifeIsAParadox LifeIsAParadox added merge conflicts This PR has merge conflicts that need solving. reviews needed This PR needs reviews and removed reviews needed This PR needs reviews merge conflicts This PR has merge conflicts that need solving. labels Oct 30, 2024
@Emirlol Emirlol changed the title Add mithril carpet highlighter Add unbreakable carpet highlighter Nov 9, 2024
@Emirlol Emirlol added the new feature This issue or PR is a new feature label Nov 9, 2024
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

It seems that this pr just continually adds to the list of carpets without checking if it was already in the list? The list should be changed to a set, (AVLTreeSet should be better in this case as this is look-up intensive.) Also set contains should be checked before calling checkForCarpet.

INSTANCE.configCallback(SkyblockerConfigManager.get().mining.dwarvenMines.carpetHighlightColor);
WorldRenderEvents.AFTER_TRANSLUCENT.register(INSTANCE::render);
SkyblockEvents.LOCATION_CHANGE.register(INSTANCE::onLocationChange);
ClientTickEvents.END_CLIENT_TICK.register(INSTANCE::tick);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could just be Scheduler#scheduleCyclic, and you would also not need the tick counter stuff.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Nov 14, 2024
@Emirlol
Copy link
Collaborator Author

Emirlol commented Nov 14, 2024

It seems that this pr just continually adds to the list of carpets without checking if it was already in the list? The list should be changed to a set, (AVLTreeSet should be better in this case as this is look-up intensive.) Also set contains should be checked before calling checkForCarpet.

CARPET_LOCATIONS is an ObjectArraySet, so it already does the contains check within its add method.
As for using an AVLTreeSet, sure, that's easy to change.

@kevinthegreat1
Copy link
Collaborator

CARPET_LOCATIONS is an ObjectArraySet, so it already does the contains check within its add method. As for using an AVLTreeSet, sure, that's easy to change.

Oh, sorry. I read it as ObjectArrayList for some reason. Then my comment about AVLTreeSet is not needed.

@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Nov 14, 2024
@Emirlol
Copy link
Collaborator Author

Emirlol commented Nov 14, 2024

Oh, sorry. I read it as ObjectArrayList for some reason. Then my comment about AVLTreeSet is not needed.

Oops too late xd

Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

LGTM.

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Nov 14, 2024
@kevinthegreat1
Copy link
Collaborator

Oops too late xd

On the bright side, we can handle millions of carpets now...

@Emirlol
Copy link
Collaborator Author

Emirlol commented Nov 14, 2024

I'm not sure how well an AVLTreeSet will handle iteration over millions of carpets per frame, though.

And I just noticed that light gray carpet is used for sneaky lighting in various places outside of tungsten ore veins. Idk what we should do for these false positives. It probably doesn't matter since they are indeed unbreakable carpets, but still.

}

public void tick() {
if (!isLocationValid || MinecraftClient.getInstance().world == null || MinecraftClient.getInstance().player == null) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also check for whether the feature is enabled or not so that we are not locating carpets unnecessarily.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed merge me please Pull requests that are ready to merge labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested This PR need changes new feature This issue or PR is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants