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

Add a detailed Mixin tutorial in advanced area. #10

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Jissee
Copy link

@Jissee Jissee commented Sep 6, 2023

I wrote a Mixin tutorial. I also warn the readers that they should use Events rather than Mixin for safety.

Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 left a 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.

@Jissee
Copy link
Author

Jissee commented Sep 7, 2023

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.

Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 left a 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.

Comment on lines +16 to +33
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
}
}
}
Copy link
Contributor

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
Copy link
Contributor

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.
Copy link
Contributor

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.

Comment on lines +38 to +40
mixin {
add sourceSets.main, "${mod_id}.refmap.json"
}
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, why?

Comment on lines +13 to +18
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.
Copy link
Contributor

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.
Copy link
Contributor

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.

Comment on lines +41 to +49
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
...
```
Copy link
Contributor

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.

Comment on lines +51 to +53
:::tip
You cannot overwrite the constructors.
:::
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain why.

Comment on lines +4 to +8
:::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.
Copy link
Contributor

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.

@Jissee
Copy link
Author

Jissee commented Sep 7, 2023

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.

@ChampionAsh5357
Copy link
Contributor

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.

@TelepathicGrunt
Copy link
Sponsor

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
https://gist.github.com/TelepathicGrunt/3784f8a8b317bac11039474012de5fb4

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.

None yet

3 participants