-
-
Notifications
You must be signed in to change notification settings - Fork 58
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 a detailed Mixin tutorial in advanced area. #10
base: main
Are you sure you want to change the base?
Conversation
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.
Currently, this does not explain anything. Rather, it mentions how to copy-paste things with a brief explanation on some of the annotations. Documentation needs to explain what something is, when you should use it, why it's useful, and how to do so in a general manner. Examples can be included, but they are meant to be supplementary to the actual explanation.
Thanks for your recommendations. I have improved and changed most of the pages I uploaded. Please review them and contact me if there are any mistakes. |
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.
More explanations are required. Also, if one thing is documented, all or at least the most common usecases should be documented as well. There are numerous things that are glossed over here that have an important reason why they were designed that way.
minecraft { | ||
runs { | ||
client { | ||
property 'forge.enabledGameTestNamespaces', mod_id | ||
args "-mixin.config=${mod_id}.mixins.json" // add this line | ||
} | ||
server { | ||
property 'forge.enabledGameTestNamespaces', mod_id | ||
args '--nogui' | ||
args "-mixin.config=${mod_id}.mixins.json" // add this line | ||
} | ||
data { | ||
workingDirectory project.file('run-data') | ||
args '--mod', mod_id, '--all', '--output', file('src/generated/resources/'), '--existing', file('src/main/resources/') | ||
args "-mixin.config=${mod_id}.mixins.json" // add this line | ||
} | ||
} | ||
} |
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.
A lot of these arguments are unnecessary. Additionally, what does it mean to specify the mixin config name? Where does the relative path point? Can a subfolder be used?
@@ -0,0 +1,148 @@ | |||
Preparation |
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.
This needs an introduction as to what mixins are. Why would I do this if I don't even know what the purpose of mixins are for?
Edit Your ```build.gradle``` | ||
---------------------- | ||
|
||
Apply the mixin plugin. |
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.
Probably should explain that the plugin is available on whatever maven is mirroring it. I imagine NeoForged unless they've added it to maven central recently.
mixin { | ||
add sourceSets.main, "${mod_id}.refmap.json" | ||
} |
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.
Why? What purpose does this serve?
```groovy | ||
dependencies { | ||
minecraft "net.neoforged:forge:${minecraft_version}-${neo_version}" | ||
annotationProcessor 'org.spongepowered:mixin:0.8.5:processor' // add this line |
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.
Again, why?
You should not consider overwriting unless: | ||
|
||
- You want to completely change vanilla mechanisms. | ||
- You want to remove some codes from these vanilla methods. | ||
- You want to fix the bugs in these vanilla methods. | ||
- You have a better implementation than these vanilla codes. |
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.
None of these are potentially good reasons. Overwrites should be used if there is no other potential alternative. You can do most of these things with injections in 90% of cases.
|
||
You can write overwriting methods in your mixin class, which will be applied to the target class. | ||
|
||
Overwriting methods are annotated with ```@Overwrite``` and they must have the same names, parameter types, and sequences as the vanilla methods. What can be changed is only the parameter names. Otherwise, the overwriting will not take effect. |
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.
This should probably explain about param names not really meaning anything in bytecode land with the LVT.
Once you have overwritten some method, you should add the JavaDoc, including the author's name and reasons, to the method. | ||
```java | ||
/** | ||
* @author | ||
* @reason | ||
*/ | ||
@Overwrite | ||
... | ||
``` |
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.
@reason
is not a valid javadoc tag by default. Also, you should always document your code regardless.
:::tip | ||
You cannot overwrite the constructors. | ||
::: |
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.
Explain why.
:::danger | ||
Before we start, we shall recommend you use [Events][events] instead of Mixin as it provides a safer way to inject your code. You should consider using Mixin if and only if you cannot find any satisfied Event and you must edit vanilla Minecraft codes. | ||
::: | ||
|
||
[SpongePowered Mixin][mixin_github] is a trait/mixin and bytecode weaving framework for Java using ASM. You can "edit" Minecraft vanilla codes or inject your own codes into Minecraft through Mixin. |
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.
This needs a better introduction on what this is.
Thanks for your concern and recommendations, which are really helpful for me. I am only a college student and a beginner at Minecraft modding actually. I wrote those documents based on my development experience. Sorry for my incomplete document contribution. I am a little tired these days and I am afraid that I cannot answer some of your questions. You can close this PR if this bothers you and I will continue to study in the future. Again, thanks for your patience. |
It's fine. If you don't have the time to contribute or address the issues, then you're welcome to leave it alone or close at a later point in time depending on if you ever wish to come back to this. |
With NeoForge packaging Fabric Mixins and Mixin Extras, the buildscript stuff isn't that much needed anymore. I guess we could still have a document explaining when one should and should use mixins and state some best practices. Sorta like this gist that says some official resources at top and walks through how to write mixins and what kinds to use and pitfalls to avoid. The part about how to dump mixin'ed classes to see their modifications would be an extremely useful thing for players and modders to know about |
I wrote a Mixin tutorial. I also warn the readers that they should use Events rather than Mixin for safety.