Repository navigation
Mixin Cleanup & Fixes - #3905
AViewFromTheTop wants to merge 1 commit into
Conversation
- Cleaned up duplicate/redundant invocations of Minecraft.getInstance() - Re-implemented a few mixin Overwrites without using Overwrite - Implemented missing Dry Foliage color optimization - Misc. cleanup
douira
left a comment
There was a problem hiding this comment.
I agree with most of the changes but I'm not quite sure about some of the changes from var to specific types. We have a lot of usage of var throughout the codebase and this is largely intentional as in many places the type is obvious or verbose. Can you explain why in these particular places you found it useful to expand to the full type?
| if (this.xScale != 1.0F || this.yScale != 1.0F || this.zScale != 1.0F) { | ||
| matrixStack.scale(this.xScale, this.yScale, this.zScale); | ||
| } | ||
| @WrapWithCondition( |
There was a problem hiding this comment.
can you explain what what this set of changes is doing and why?
There was a problem hiding this comment.
This is re-implementing the Overwrite, but using MixinExtras functionality in its place. I don’t like the conflict-heavy nature of Overwrite, but I also honestly didn’t expect this change in particular to be well-accepted.
| } | ||
|
|
||
| var sb = new StringBuilder(); | ||
| StringBuilder builder = new StringBuilder(); |
There was a problem hiding this comment.
why is this necessary or better?
|
|
||
| private static void printCompatibilityReport(Collection<ScannedResourcePack> scanResults) { | ||
| var builder = new StringBuilder(); | ||
| StringBuilder builder = new StringBuilder(); |
There was a problem hiding this comment.
is this better or necessary?
|
We prefix mixin methods with
|
Simply for readability ^^ |
|
Well changing all the var usage would require much more than just these changes so it seemed arbitrary to me what you changed just these instances and thus assumed there was some more concrete justification. I don't care whether you revert it, since we don't apply var everywhere. |
I personally would argue for altering the codestyle of the mod altogether in some places if that's something the Sodium team is interested in, there are many places it can improve readability (especially mixin parameters/AT annotations.)
I have also noticed that many classes that only provide static methods aren't marked as final and don't have private constructors, and there is major inconsistency between whether methods, new fields, new methods, etc., in mixins are prefixed with "sodium$" or not, as wall as frequent usage of "var" for local variables instead of providing their proper types. For the sake of consistency and readability for new contributors, I would advocate for a consensus on these too!
As I am not a member of the Sodium team I opted to not alter the codestyle much, and only made changes where it seemed to benefit consistency given what was already there.
On a side-note, the name of this pull request is likely sub-optimal. I'm not sure what it should be changed to, but this description should explain it well enough.