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

Draft: Feats UI/1.18 #212

Merged
merged 116 commits into from
Sep 24, 2023
Merged

Draft: Feats UI/1.18 #212

merged 116 commits into from
Sep 24, 2023

Conversation

Uhutown
Copy link
Collaborator

@Uhutown Uhutown commented Aug 15, 2023

No description provided.

Copy link
Owner

Choose a reason for hiding this comment

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

getInfo should be cached and

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's getting chached in the SuperClass ContainerBase. Why should we chache it two times?

Copy link
Owner

Choose a reason for hiding this comment

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

  1. No the function call it self is not it is still entering into a function call every time. Means function call overhead in EVERY line
  2. If you need to change things it makes it harder. What if you make the function more complex? Even only by accident?

Copy link
Owner

Choose a reason for hiding this comment

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

getInfo local cache

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The same as abow

Copy link
Owner

Choose a reason for hiding this comment

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

getinfo local cache

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The same

private static final float[] ALL_LINES = getLines();
private static final int TILE_WIDTH = 10;
private static final int TILE_COUNT = 100;
public static final float[] ALL_LINES = getLines();
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this line cache public now? Access should be limited as the contents might be invalidated

@@ -122,9 +129,10 @@ public void update(final String errorString) {
}

private void resetTileSelection() {
this.entity.findRecursive(UIColor.class).stream()
.filter(color -> color.getColor() == SELECTION_COLOR)
this.entity.findRecursive(UIColor.class).stream().filter(
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah so this should be done differently if we are honest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How?

Copy link
Owner

Choose a reason for hiding this comment

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

Cache the UIColors for example idk thing of something

Copy link
Owner

Choose a reason for hiding this comment

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

getInfo should be localy cached

@@ -21,7 +21,7 @@ public class ClientNameHandler implements INetworkSync {

public static String getClientName(final NameStateInfo info) {
synchronized (CLIENT_NAMES) {
return CLIENT_NAMES.getOrDefault(info, "");
return new String(CLIENT_NAMES.getOrDefault(info, ""));
Copy link
Owner

Choose a reason for hiding this comment

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

Why is there a string copy? Strings are immutable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I wasn't if this is so

@@ -27,7 +27,7 @@ public class ClientSignalStateHandler implements INetworkSync {

public static final Map<SEProperty, String> getClientStates(final ClientSignalStateInfo info) {
synchronized (CURRENTLY_LOADED_STATES) {
return CURRENTLY_LOADED_STATES.computeIfAbsent(info, _u -> new HashMap<>());
return CURRENTLY_LOADED_STATES.getOrDefault(info, new HashMap<>());
Copy link
Owner

Choose a reason for hiding this comment

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

Why was this changed unneccessary hash map allocation on all accesses

@@ -335,6 +340,41 @@ public Optional<Point> tryReset(final BlockPos position) {
return Optional.of(point);
}

private boolean checkReverseReset(final BlockPos pos) {
Copy link
Owner

Choose a reason for hiding this comment

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

What is this trying to achieve?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for the revers pathway reset.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I can read but what is this doing and why are we in linear time here what exactly do you mean by reverse reset

@MrTroble MrTroble self-requested a review September 24, 2023 18:41
@Uhutown Uhutown merged commit 41a638c into 1.18-master Sep 24, 2023
5 checks passed
@Uhutown Uhutown deleted the feats_UI/1.18 branch September 24, 2023 18:45
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.

4 participants