-
-
Notifications
You must be signed in to change notification settings - Fork 421
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
Added NoFog #904
Added NoFog #904
Conversation
WalkthroughWalkthroughThe changes involve the introduction of a new feature called Changes
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 X ? TipsChat with CodeRabbit Bot (
|
There was a problem hiding this 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
Files ignored due to filter (1)
- src/main/resources/assets/wurst/lang/en_us.json
Files selected for processing (3)
- src/main/java/net/wurstclient/hack/HackList.java (1 hunks)
- src/main/java/net/wurstclient/hacks/NoFogHack.java (1 hunks)
- src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (2 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/net/wurstclient/hacks/NoFogHack.java
Additional comments: 4
src/main/java/net/wurstclient/hack/HackList.java (1)
- 134-140: The addition of
NoFogHack
to theHackList
class is consistent with the description of the new feature. It's important to ensure that theNoFogHack
class is properly implemented and follows the same conventions as the other hack classes in the project. Additionally, the initialization of this hack should be tested to ensure it behaves as expected within the game environment.src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (3)
7-17: The import statement for
net.minecraft.client.render.Camera
is correctly added to support the new functionality introduced in the mixin. Ensure that there are no unused imports after this addition.22-32: The
onApplyFog
method is correctly using the@Inject
annotation to intercept the fog application process. The method checks ifNoFogHack
is enabled and cancels the fog rendering if it is. This is a good use of mixins to modify game behavior without altering the original game code. However, ensure that theWurstClient.INSTANCE.getHax().noFogHack
is thread-safe since Minecraft is a multi-threaded application and this could potentially be accessed from multiple threads simultaneously.34-36: The
onGetFogModifier
method is not part of the changes described in the summary, but it seems to be related to a similar feature,antiBlindHack
. If this is an intended change, ensure that it is documented in the pull request summary. If it's not related to theNoFogHack
, it should be reviewed separately to ensure it's part of a relevant change set.
Good AI |
There was a problem hiding this 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
Files selected for processing (2)
- src/main/java/net/wurstclient/hacks/NoFogHack.java (1 hunks)
- src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/net/wurstclient/hacks/NoFogHack.java
Additional comments: 1
src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (1)
- 25-34: The implementation of the
onApplyFog
method correctly cancels the fog application whennoFogHack
is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there, @IUDevman. Thanks for the pull request and your effort to contribute to the Wurst Client.
I've taken a look at your implementation for the NoFog hack, and I've encountered a few issues. Your approach has an unfortunate side effect in the form of a visual artifact: it turns the bottom half of the skybox into the fog color, creating a sharp line in the middle.
Additionally, the way you've addressed the removal of blindness and underwater fog seems to conflict with the existing AntiBlind and NoOverlay hacks. If a user enables NoFog but doesn't enable AntiBlind or NoOverlay, they likely still want those effects to be present.
Also, both AntiBlind and NoOverlay manage these effects in a different way than simply deactivating the fog, which makes it look a bit strange visually when NoFog is used instead.
AntiBlind / NoOverlay | NoFog | |
---|---|---|
Blindness | ||
Underwater |
As you can see, the rendering with NoFog doesn't quite match up to the other hacks' performance. I think the best way to address this would be to make NoFog leave these kinds of fog alone and focus solely on regular distance fog.
I think your NoFog hack can still be a great addition once these issues are addressed. Please take these points into consideration and push the improvements to your pull request.
Hi Alexander, thanks for the review. I will work on taking your points in consideration and have NoFog work for only regular distance fog. As for the skybox, that might be a bit more challenging (its always there vanilla, fog hides it) but I should be able to find a solution. Thank you again. |
Still working on the skybox
There was a problem hiding this 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
Files ignored due to filter (1)
- src/main/resources/assets/wurst/lang/en_us.json
Files selected for processing (1)
- src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (2 hunks)
Additional comments: 4
src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (4)
10-18: The import statements are correctly added to support the new methods introduced in the mixin.
29-34: The
getFogModifier
method is correctly implemented as a shadow method, returning null.36-53: The
onApplyFog
method is correctly implemented to support the NoFog feature by checking thenoFogHack
status and cancelling the fog application under the right conditions.23-57: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [55-61]
The
onGetFogModifier
method is correctly implemented to support the AntiBlind feature by checking theantiBlindHack
status and cancelling the fog modifier if it is enabled.
This implementation cancels the distance fog while leaving other features such as blindness, fluid fog, and powdered snow effect intact. Still working on that skybox. |
There was a problem hiding this 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
Files selected for processing (1)
- src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (2 hunks)
Additional comments: 2
src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (2)
26-55: The
onApplyFog()
method correctly implements the NoFog feature by checking ifnoFogHack
is enabled and then preventing fog application under certain conditions. This aligns with the pull request description.57-59: The
onGetFogModifier()
method appears to be related to theantiBlindHack
rather than the NoFog feature. Ensure that this change is intentional and correctly placed within this pull request, as it is not mentioned in the summary of changes.
I finally found the issue. It seems minecraft uses the fog rendering to add shading from the void, sunset, etc. Removing the opacity on the color instead of outright cancelling the render allows the shading to still be in place. In my research I saw that other mods push the fog rendering out, but that is still shown at a really far render distance and causes the same skybox problem. |
There was a problem hiding this 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
Files selected for processing (1)
- src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (2 hunks)
Additional comments: 4
src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (4)
7-21: The added imports are necessary for the new functionality introduced in the mixin and are correctly placed at the top of the file.
30-35: Verify that the
getFogModifier
method is intended to always return null, as this could potentially override behavior from the originalBackgroundRenderer
class.37-53: Ensure that
WurstClient.INSTANCE.getHax().noFogHack
is properly initialized before this mixin is loaded to prevent potential null pointer exceptions.55-57: Confirm that the
antiBlindHack
andnoFogHack
features do not conflict with each other, as they both affect fog rendering.
There was a problem hiding this 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
Files ignored due to filter (1)
- src/main/resources/assets/wurst/lang/en_us.json
Files selected for processing (2)
- src/main/java/net/wurstclient/hack/HackList.java (1 hunks)
- src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (1 hunks)
Additional comments: 7
src/main/java/net/wurstclient/hack/HackList.java (1)
- 137-137: The addition of
NoFogHack
to theHackList
class appears to be implemented correctly and follows the established pattern for hack declarations.src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (6)
38-56: The implementation of
onApplyFog
seems to correctly check if theNoFogHack
is enabled and sets the fog color to transparent if certain conditions are met. However, it's important to ensure that theRenderSystem.setShaderFogColor(0, 0, 0, 0)
call does not have any unintended side effects on the rendering system, as it sets the fog color to fully transparent.58-66: The
onGetFogModifier
method correctly cancels the fog modifier when theantiBlindHack
is enabled. It's important to verify that this does not interfere with other game mechanics that might rely on the fog modifier, and that theantiBlindHack
is intended to cancel all fog modifiers, not just those related to blindness effects.43-43: Verify that the
NoFogHack
is properly initialized and that itsisEnabled
method accurately reflects whether the hack is active. This is crucial for the correct functioning of theonApplyFog
method.64-64: Ensure that the
antiBlindHack
is correctly registered and that itsisEnabled
method is reliable, as theonGetFogModifier
method depends on this to cancel the fog modifier.38-41: Confirm that the method signature for
onApplyFog
matches the signature of the method it is injecting into. This is important for the mixin to function correctly.58-61: Verify that the method signature for
onGetFogModifier
is correct and matches the method it is supposed to inject into. This ensures that the mixin works as intended.
There was a problem hiding this 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
Files selected for processing (2)
- src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java (1 hunks)
- src/main/resources/wurst.accesswidener (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/net/wurstclient/mixin/BackgroundRendererMixin.java
Additional comments: 1
src/main/resources/wurst.accesswidener (1)
- 4-4: The addition of the
getFogModifier
method to the access widener file is consistent with the PR's objective to implement the NoFog feature by altering fog rendering behavior.
There was a problem hiding this 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
Files selected for processing (1)
- src/main/java/net/wurstclient/hacks/NoFogHack.java (1 hunks)
Additional comments: 3
src/main/java/net/wurstclient/hacks/NoFogHack.java (3)
8-24: The
NoFogHack
class is correctly defined in thenet.wurstclient.hacks
package and extends theHack
base class. The use of@SearchTags
is appropriate for discoverability of the feature within the client.17-21: The constructor of
NoFogHack
is well-defined, setting the name and category of the hack as expected.23-23: The comment referencing
BackgroundRendererMixin
provides a useful pointer to the related mixin functionality, which is a good practice for code maintainability.
Description
New hack - NoFog
This hack removes fog from the world. This includes fog for the background, when in fluids, blindness effect, and packed snow effect.