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

NameTags fixes #798

Merged
merged 8 commits into from
Dec 27, 2023
Merged

Conversation

ThisTestUser
Copy link
Contributor

@ThisTestUser ThisTestUser commented Mar 14, 2023

-Make nametags see through option work well everywhere (allowing it to be enabled by default)
-Added an option to force show mob nametags (different from force showing player nametags)

Before:
Before

After:
After

-Make nametags see through option work well everywhere (allowing it to be enabled by default)
-Added an option to force show mob nametags
@github-actions
Copy link

This pull request has been open for a while with no recent activity. If you're still working on this or waiting for a review, please add a comment or commit within the next 7 days to keep it open. Otherwise, the pull request will be automatically closed to free up time for other tasks.

Pull requests should be closed if:

  • They have been superseded by another pull request
  • They are out of scope or don't align with the project
  • They have become obsolete due to other changes
  • They have bugs or conflicts that won't be resolved

@coderabbitai
Copy link

coderabbitai bot commented Oct 22, 2023

Walkthrough

Walkthrough

The recent changes aim to enhance the Wurst Client by introducing a new feature that enables users to have separate control over named mobs and player names. This involves updating the NameTagsHack class to include the seeThrough setting's impact on graphical rendering, splitting the forceNametags setting into forceMobNametags and forcePlayerNametags, and modifying the rendering logic in EntityRendererMixin, MobEntityRendererMixin, and LivingEntityRendererMixin to accommodate these changes.

Changes

File Path Summary
src/main/java/net/wurstclient/hacks/NameTagsHack.java Updated seeThrough setting description. Split forceNametags into forceMobNametags and forcePlayerNametags. Included shouldForceMobNametags and shouldForcePlayerNametags methods.
src/main/java/net/wurstclient/mixin/EntityRendererMixin.java,
src/main/java/net/wurstclient/mixin/MobEntityRendererMixin.java,
src/main/java/net/wurstclient/mixin/LivingEntityRendererMixin.java
Modified rendering logic to accommodate changes in NameTagsHack class.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • You can tag CodeRabbit on specific lines of code or entire files in the PR by tagging @coderabbitai in a comment.
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • You can tag @coderabbitai in a PR comment and ask questions about the PR and the codebase. Examples:
    • @coderabbitai generate interesting stats about this repository.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid.
    • @coderabbitai read the files in the src/scheduler package and generate README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between b1076bd and db73bd8.
Files ignored due to filter (1)
  • src/main/resources/wurst.mixins.json
Files selected for processing (3)
  • src/main/java/net/wurstclient/hacks/NameTagsHack.java (2 hunks)
  • src/main/java/net/wurstclient/mixin/EntityRendererMixin.java (1 hunks)
  • src/main/java/net/wurstclient/mixin/MobEntityRendererMixin.java (1 hunks)
Additional comments (Suppressed): 8
src/main/java/net/wurstclient/mixin/MobEntityRendererMixin.java (1)
  • 19-37: The mixin implementation for forcing mob nametags seems correct. However, it only forces the nametag if the shouldForceMobNametags() method returns true. This method should also check if the mob has a custom name, as mentioned in the comment on line 33, but it doesn't appear to do so. Please verify if this is the intended behavior.
		// return true if mob has custom name
		if(WurstClient.INSTANCE.getHax().nameTagsHack.shouldForceMobNametags())
+               && mobEntity.hasCustomName())
			cir.setReturnValue(true);
src/main/java/net/wurstclient/mixin/EntityRendererMixin.java (1)
  • 96-100: The new condition !nameTags.isSeeThrough() has been added to the ternary operator that determines the TextLayerType. This change allows the text layer to be see-through only when the entity is not sneaky and the see-through option is not enabled in the NameTags settings. This is a logical change that improves the flexibility of the NameTags feature. However, please verify that this change does not conflict with other parts of the code that rely on the TextLayerType.
src/main/java/net/wurstclient/hacks/NameTagsHack.java (6)
  • 22-26: The default value for the seeThrough setting has been changed from false to true. Ensure that this change is intentional and that it does not negatively impact the user experience. Also, the description of the setting has been simplified, removing the mention of potential readability issues behind water and other transparent things. If this potential issue still exists, it might be worth keeping the original, more detailed description.

  • 28-30: No significant changes in the forceNametags setting. The original functionality and description are preserved.

  • 32-34: A new setting forceMobNametags has been introduced. This setting allows users to force the visibility of mob nametags. The implementation seems correct based on the provided context.

  • 36-44: The new forceMobNametags setting is added to the NameTagsHack constructor. This ensures that the setting is initialized when the hack is created. The order of adding settings is preserved, which is good for consistency.

  • 61-64: A new method shouldForceMobNametags() has been added. This method checks if the hack is enabled and if the forceMobNametags setting is checked. The implementation is consistent with the existing methods for checking settings.

  • 66-68: The comment has been updated to include the MobEntityRendererMixin. This suggests that the new forceMobNametags setting is likely implemented in this mixin. Without the actual mixin code, it's hard to verify this, but the comment update is consistent with the changes made in this hunk.

Copy link

This pull request has been open for a while with no recent activity. If you're still working on this or waiting for a review, please add a comment or commit within the next 7 days to keep it open. Otherwise, the pull request will be automatically closed to free up time for other tasks.

Pull requests should be closed if:

  • They have been superseded by another pull request
  • They are out of scope or don't align with the project
  • They have become obsolete due to other changes
  • They have bugs or conflicts that won't be resolved

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between db73bd8 and 993aa80.
Files ignored due to filter (1)
  • src/main/resources/wurst.mixins.json
Files selected for processing (2)
  • src/main/java/net/wurstclient/hacks/NameTagsHack.java (2 hunks)
  • src/main/java/net/wurstclient/mixin/EntityRendererMixin.java (1 hunks)
Additional comments: 4
src/main/java/net/wurstclient/hacks/NameTagsHack.java (3)
  • 22-27: The updated description for the seeThrough setting now accurately reflects the potential for graphical glitches with transparent elements, which is a good practice for user awareness.

  • 33-35: The addition of the forceMobNametags setting is consistent with the PR objectives to allow forced visibility of mob nametags. Ensure that this new setting is integrated properly throughout the application.

  • 62-64: The method shouldForceMobNametags correctly checks if the hack is enabled and if the forceMobNametags setting is checked. This logic aligns with the intended functionality of the new setting.

src/main/java/net/wurstclient/mixin/EntityRendererMixin.java (1)
  • 95-100: The logic to determine the TextLayerType based on the nameTags.isSeeThrough() condition has been updated. This change should enable the use of a see-through text layer when configured, which is in line with the PR objectives. Verify that this change does not introduce any rendering issues with other text layers.

@Alexander01998 Alexander01998 added type:enhancement New feature or request area:settings Improves the settings of existing features. category:render labels Dec 27, 2023
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 993aa80 and 02a2e38.
Files selected for processing (4)
  • src/main/java/net/wurstclient/hacks/NameTagsHack.java (2 hunks)
  • src/main/java/net/wurstclient/mixin/EntityRendererMixin.java (1 hunks)
  • src/main/java/net/wurstclient/mixin/LivingEntityRendererMixin.java (1 hunks)
  • src/main/java/net/wurstclient/mixin/MobEntityRendererMixin.java (1 hunks)
Additional comments: 5
src/main/java/net/wurstclient/mixin/MobEntityRendererMixin.java (1)
  • 19-37: The mixin correctly overrides the default behavior to always show mob nametags when the shouldForceMobNametags setting is enabled. This change aligns with the PR objectives to allow forced display of mob nametags.
src/main/java/net/wurstclient/mixin/LivingEntityRendererMixin.java (1)
  • 49-54: The change to call shouldForcePlayerNametags instead of shouldForceNametags is consistent with the PR objectives to separate the control over player and mob nametags. Ensure that this change is reflected across all relevant parts of the codebase where player nametag behavior is determined.
Verification successful

The search for the method shouldForceNametags did not yield any results in the src/main/java/net/wurstclient/ directory. This suggests that the method shouldForcePlayerNametags has replaced shouldForceNametags where necessary in the context of the LivingEntityRendererMixin.java. However, without further context, we cannot be certain that this change is comprehensive across the entire codebase.

Given the lack of output, it's prudent to perform an additional check to ensure that the new method shouldForcePlayerNametags is indeed present and used correctly in the NameTagsHack class, as this is central to the PR objectives.


The absence of output from the ast-grep command suggests that the shouldForcePlayerNametags method may not be present in the NameTagsHack class, or the pattern provided to ast-grep did not match the actual structure of the class. This could indicate a potential issue, as the PR objectives mention the introduction of this method.

To ensure that the shouldForcePlayerNametags method exists and is implemented correctly, we should attempt a different approach to verify its presence in the NameTagsHack class. Let's use rg to search for the method definition within the Java files.


The output from the rg command confirms the presence of the shouldForcePlayerNametags method within the NameTagsHack class in the src/main/java/net/wurstclient/hacks/NameTagsHack.java file. This aligns with the PR objectives, which mention the introduction of this method to handle the forced display of player nametags separately from mob nametags.

Given this confirmation, the change in the LivingEntityRendererMixin.java to call shouldForcePlayerNametags instead of shouldForceNametags is verified to be consistent with the intended functionality described in the PR.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for other occurrences where shouldForceNametags might still be used.
rg --type java 'shouldForceNametags' src/main/java/net/wurstclient/

Length of output: 67



Script:

#!/bin/bash
# Verify the presence and usage of shouldForcePlayerNametags in the NameTagsHack class.
ast-grep --lang java --pattern 'class NameTagsHack {
  $$$
  boolean shouldForcePlayerNametags() {
    $$$
  }
  $$$
}'

Length of output: 121



Script:

#!/bin/bash
# Search for the definition of shouldForcePlayerNametags in the NameTagsHack class.
rg --type java 'boolean shouldForcePlayerNametags\(\)' src/main/java/net/wurstclient/

Length of output: 185

src/main/java/net/wurstclient/hacks/NameTagsHack.java (2)
  • 22-47: The split of forceNametags into forceMobNametags and forcePlayerNametags provides more granular control as per the PR objectives. The updated description for the seeThrough setting clarifies its impact on rendering, which is a good practice for user-facing features.

  • 60-67: The methods shouldForceMobNametags and shouldForcePlayerNametags are correctly implemented to check the state of their respective settings. This change supports the new feature addition and improvement mentioned in the PR objectives.

src/main/java/net/wurstclient/mixin/EntityRendererMixin.java (1)
  • 93-108: The logic to adjust the text layers based on the seeThrough setting is implemented correctly. However, ensure that the graphical glitches mentioned in the setting's description are acceptable or mitigated. This change aligns with the PR objectives to enhance the see-through option for nametags.

@Alexander01998 Alexander01998 merged commit ba7d2aa into Wurst-Imperium:master Dec 27, 2023
3 checks passed
@Alexander01998 Alexander01998 added this to the v7.41 milestone Dec 27, 2023
@Alexander01998 Alexander01998 added the status:merged This pull request has been merged, even if GitHub says otherwise. label Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:settings Improves the settings of existing features. category:render status:merged This pull request has been merged, even if GitHub says otherwise. type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants