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 Speed Presets for configurable speed settings #1111

Merged
merged 18 commits into from
Dec 30, 2024

Conversation

Manchick0
Copy link
Contributor

Introduced a Speed Presets system with customizable presets for the rancher boots. Added a GUI to manage presets and integrated support for the setmaxspeed command, and the sign-editor to use these presets.

Besides all of that, refactored item protection rendering logic to show it everywhere, where applicable, and render it in front of the item, as well as updated the texture being used.

Introduced a Speed Presets system with customizable presets for the rancher boots. Added a GUI to manage presets and integrated support for the `setmaxspeed` command, and the sign-editor to use these presets.

Besides all of that, refactored item protection rendering logic to show it everywhere, where applicable, and render it in front of the item, as well as updated the texture being used.
@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Dec 28, 2024
I apologize for the changes, since I didn't know that the texture was applied elsewhere. This commit reverts all the changes to the size of the texture and the way it is rendered, to hopefully prevent any issues.
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.

I don't see the item protection changes mentioned in your description. Also please separate the item protection changes and texture changes to a separate pr.

Comment on lines 14 to 15

@ModifyExpressionValue(method = "drawCooldownProgress", at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/player/ItemCooldownManager;getCooldownProgress(Lnet/minecraft/item/ItemStack;F)F"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's literally nothing changed here?

Comment on lines 100 to 101
@Shadow
private boolean touchIsRightClickDrag;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this shadowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question...

*
* @return true if the state has been modified; false otherwise.
*/
public boolean hasBeenChanged() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic has some problems, where an entry changed and then changed back to the original would still be marked as changed. In general, you need a validation method that checks if all entries are the same. I'd suggest looking at ShortcutsConfigListWidget#hasChanges.

Comment on lines 53 to 65
var presets = SpeedPresets.getInstance().getPresetCount();
// Accounting 1 for the title entry.
var children = children().size() - 1;
if (children < presets) return true;

children = 0;
for (var child : children()) {
if (child.hasBeenModified()) return true;
if (child instanceof SpeedPresetEntry entry) {
children = entry.isEmpty() ? children : children + 1;
}
}
return children != presets;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var presets = SpeedPresets.getInstance().getPresetCount();
// Accounting 1 for the title entry.
var children = children().size() - 1;
if (children < presets) return true;
children = 0;
for (var child : children()) {
if (child.hasBeenModified()) return true;
if (child instanceof SpeedPresetEntry entry) {
children = entry.isEmpty() ? children : children + 1;
}
}
return children != presets;
return children().stream().anyMatch(AbstractEntry::hasBeenModified) || children().stream().filter(SpeedPresetEntry.class::isInstance).map(SpeedPresetEntry.class::cast).filter(entry -> !entry.isEmpty()).count() != SpeedPresets.getInstance().getPresetCount();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This results into two separate loops although it could be done it one

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Dec 29, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really good api here btw.

@Manchick0
Copy link
Contributor Author

I don't see the item protection changes mentioned in your description. Also please separate the item protection changes and texture changes to a separate pr.

Yeah, I shouldn't have included it in this PR in the first place, but I'll try to revert the changes and use the old texture

@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Dec 29, 2024
Manchick0 and others added 13 commits December 29, 2024 23:40
Removed the `hasBeenModified` flag, and replaced it with direct comparisons of initial and current values.
Manchick0 and others added 2 commits December 30, 2024 00:30
Replaced LinkedHashMap with Object2IntMap for efficiency and consistency in managing speed presets. Adjusted related methods and logic to accommodate the new data structure. Updated SpeedPresetListWidget to handle changes and comparisons using the revised map implementation.
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, thanks for contributing!

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Dec 29, 2024
@kevinthegreat1 kevinthegreat1 merged commit 19a1df6 into SkyblockerMod:master Dec 30, 2024
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Dec 30, 2024
@AzureAaron AzureAaron added the new feature This issue or PR is a new feature label Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This issue or PR is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants