Rewrite #26

ghost · 2023-05-12T16:24:38Z

This template has become a bit outdated for my usage, so I decided to rewrite it. While the new template works, it needs better testing, please do not merge this right away if you decide to.

Features added:

Full support for Quilt
Example mixins (all platforms), example common mixin applied to all of them
Access wideners for Fabric and Quilt (disabled by default)

Changes:

Updated Gradle to 8.1.1
Updated plugins and dependencies
Mod domain can now be changed from properties
Ability to change Java version (technical, for older MC versions)

Fixes:

Some boolesn properties not actually working (were always acting as true)
Common sources and Javadoc were not added to mod-sources and mod-javadoc jars respectively

NYI/To do:

CI
Tweak Maven publishing (script needs updating)
Figure out what to do with different mappings support

jaredlll08

Preamble

Just a few things before I get into the review:

This template is open source, it is meant to be forked and changed to each author's liking, if you don't like how I am doing something in this template, then please fork it, make your changes and build your mod off your fork.
My lack of comment on any specific change does not indicate approval of said change, there are a lot of changes here that I am not a fan of, but the only comment I can add to them is "I don't like this", which I don't feel is conductive.
I know the template needs some love, it is hard finding time to update it, I'll try and find time after the 1.20 port, or when ForgeGradle works on Gradle 8.
I did not look at anything regarding quilt. This template is meant to provide a quick and easy way for you to write code once, and use it on multiple modloaders without having to maintain multiple repositories or branches. So with that in mind, adding quilt seems like it deviates from the purpose of this template, as quilt likes to point out very often, nearly all fabric mods run on quilt¹ and quilt has no plans to change that any time soon², so when I look at it, I do not see a single reason³ to use maintain both fabric and quilt, when you can maintain fabric and have it just work on quilt.

Review

There is a lot of changes that are just plain out wrong and will cause issues as soon as you try and load the template, like changing compileOnly to implementation for adding the Common project as a dependency of the Forge project, as well as adding support for Fabric to use different type of mappings without those being supported by Forge or Vanilla.

Some changes may have benefit such as adding datagen, however I don't think that really suits this template, not all mods have the need for datagen (I have personally only used it maybe twice in my mods) and not even the Forge MDK or Fabric example mod include it.

I appreciate that you went out of your way to try and improve the template, and I understand that this has been submitted as a draft PR, but with the changes you have made so far, I'm not sure that the end result will be a good fit for this template.

You can read more about the mods that don't here ↩
https://forum.quiltmc.org/t/are-there-any-plans-about-ending-compatibility-for-fabric/284/2 ↩
Yes I have been told that QSL has a better API than FAPI, but in the context of a multi mod-loader template like this, having a better API doesn't mean the API can be used, unless the QSL APIs have an equivalent API for Forge and Fabric, then using it will lead to your mod having different features on different modloaders, which is not good for users. ↩

Common/build.gradle

jaredlll08 · 2023-05-12T18:56:06Z

Common/build.gradle


-    def buildProps = project.properties.clone()
+processResources {
+    def buildProps = new HashMap<>(project.properties)


How is this better than project.properties.clone()?

jaredlll08 · 2023-05-12T18:56:56Z

Common/src/main/java/com/example/examplemod/CommonInit.java

@@ -8,24 +8,24 @@
 // import and access the vanilla codebase, libraries used by vanilla, and optionally third party libraries that provide
 // common compatible binaries. This means common code can not directly use loader specific concepts such as Forge events
 // however it will be compatible with all supported mod loaders.
-public class CommonClass {
+public class CommonInit {


Why the name change?

Common/src/main/java/com/example/examplemod/CommonInit.java

jaredlll08 · 2023-05-12T19:04:32Z

Common/src/main/java/com/example/examplemod/Constants.java

+	// This logger is used to write text to the console and the log file.
+	// It is considered best practice to use your mod id as the logger's name.
+	// That way, it's clear which mod wrote info, warnings, and errors.
+	public static final Logger LOGGER = LoggerFactory.getLogger(MOD_ID);


Why the name change?

gradle.properties

gradle/wrapper/gradle-wrapper.properties

settings.gradle

jaredlll08 · 2023-05-12T20:04:32Z

Fabric/build.gradle

+    switch (mappings_type) {
+        case "official" :
+            mappings loom.officialMojangMappings()
+            break
+        case "parchment" :
+            mappings loom.layered() {
+                officialMojangMappings()
+                parchment("org.parchmentmc.data:parchment-${parchment_mc_version}:${parchment_mappings}@zip")
+            }
+            break
+        case "yarn" :
+            mappings "net.fabricmc:yarn:${minecraft_version}+${yarn_mappings}:v2"
+            break
+        case "quilt" :
+            mappings "org.quiltmc:quilt-mappings:$minecraft_version+$quilt_mappings_version:intermediary-v2"
+            break
+        case "quilted_official" :
+            mappings loom.layered {
+                mappings "org.quiltmc:quilt-mappings:$minecraft_version+$quilt_mappings_version:intermediary-v2"
+                officialMojangMappings()
+            }
+            break
+        default :
+            mappings loom.officialMojangMappings()
+            break
+    }


This whole section of code is never going to work until VanillaGradle supports different mapping types besides mojmap, and I don't believe that will be happening anytime soon.

We have no plans to change the common project to Loom.

VanillaGradle just works, I have gotten reports that when people try and use loom, it may work for a bit but they always run into issues, but we have had no issues using VanillaGradle.

jaredlll08 · 2023-05-12T21:54:41Z

As for Quilt - I knew that most Fabric mods work with it. The reason for adding support for it is that mod authors might want their mod to behave differenlty specifically on Quilt, be it using its APIs or not. For example due to a bug that is present only on one of the platforms.

In those cases, it should be up to the mod authors to decide what they want to do, if they want to add quilt to the environment, report to quilt / fabric to get the bug fixed or to just not support quilt.

I feel adding quilt when it is not necessarily needed, will lead to extra work for the majority of mod authors supporting both, when in most cases, they should report that bug to quilt, as it's in their best interest to make sure that they don't introduce bugs in fabric mods.

Just note that as I said, I will be cleaning the template up a bit when ForgeGradle 6 releases and we can use Gradle 8, so I don't want you to waste your time now changing things when I will change them again after that happens.

Also we do have a discord server if you want to come and discuss your changes with us there:
https://discord.myceliummod.network

jaredlll08 · 2023-05-13T13:40:00Z

And I am thinking to making Quilt and option disabled by default. It is way easier for mod devs to delete unneeded files compared to adding Quilt support from scratch and tweaking it so it works. Everything should be in place is case one needs it.

To put it plain and simple, quilt is not going to be added to this base template until quilt has a valid reason to be added, which as detailed in my initial reply, I just don't see a reason for it.

It especially doesn't help that quilt-loom and fabric-loom are incompatible with each other (As you said above).

Saying that, we do already have have an examples branch that features quilt https://github.com/jaredlll08/MultiLoader-Template/tree/1.18.2/examples/MultiLoader-Template-with-quilt

If someone wanted to specifically target quilt, they can use that example as a base, but including it in this base template is going to lead to extra work for the majority of modders who do not have a reason to use quilt.

jaredlll08 · 2023-05-17T20:06:27Z

gradle.properties

@@ -28,6 +28,7 @@ common_add_to_javadoc=true
 forge_version=44.1.23
 forge_ats_enabled=false
 forge_extra_logging=false
+systemProp.net.minecraftforge.gradle.check.gradle=false


It is my understanding that Forge offers zero support for running their plugin on unsupported gradle versions, this is not something I want to ship to end users.

Darkhax · 2023-05-17T20:09:06Z

Common/src/main/java/com/example/examplemod/CommonInit.java

@@ -8,7 +8,7 @@
 // import and access the vanilla codebase, libraries used by vanilla, and optionally third party libraries that provide
 // common compatible binaries. This means common code can not directly use loader specific concepts such as Forge events
 // however it will be compatible with all supported mod loaders.
-public class CommonInit {
+public class  CommonInit {


Why are you adding double spacing here, and in other classes?

jaredlll08 · 2023-11-11T08:38:33Z

I'm going to close this PR as the template has changed a bit since it was made and would most likely require some rewriting of this PR.

Feel free to make a new PR if you update the PR.

Nik-Kot added 2 commits April 17, 2023 16:21

Rewrite

4642c31

Move Mixin version to properties

bb59c3c

ghost marked this pull request as draft May 12, 2023 16:28

ghost mentioned this pull request May 12, 2023

Parchment mappings #13

Closed

jaredlll08 reviewed May 12, 2023

View reviewed changes

Nik added 2 commits May 13, 2023 13:59

Merge branch '1.19.3' into 1.19.3

ff26757

Fix some of the mistakes pointed out in the review

92f37e3

Update to Gradle 8.1.1, ForgeGradle can be made to work with it

8e8ebcd

jaredlll08 reviewed May 17, 2023

View reviewed changes

Darkhax reviewed May 17, 2023

View reviewed changes

Nik-Kot added 2 commits May 17, 2023 23:31

Add disclaimer to ForgeGradle "Ignore Gradle version" setting

06c7bb1

Fix spacing

65bb871

jaredlll08 closed this Nov 11, 2023

ghost deleted the 1.19.3 branch March 5, 2024 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite #26

Rewrite #26

ghost commented May 12, 2023 •

edited by ghost

Loading

jaredlll08 left a comment •

edited

Loading

jaredlll08 May 12, 2023

jaredlll08 May 12, 2023

jaredlll08 May 12, 2023

jaredlll08 May 12, 2023 •

edited

Loading

jaredlll08 May 13, 2023

jaredlll08 commented May 12, 2023

jaredlll08 commented May 13, 2023

jaredlll08 May 17, 2023

Darkhax May 17, 2023

jaredlll08 commented Nov 11, 2023

Rewrite #26

Rewrite #26

Conversation

ghost commented May 12, 2023 • edited by ghost Loading

jaredlll08 left a comment • edited Loading

Choose a reason for hiding this comment

Preamble

Review

Footnotes

jaredlll08 May 12, 2023

Choose a reason for hiding this comment

jaredlll08 May 12, 2023

Choose a reason for hiding this comment

jaredlll08 May 12, 2023

Choose a reason for hiding this comment

jaredlll08 May 12, 2023 • edited Loading

Choose a reason for hiding this comment

jaredlll08 May 13, 2023

Choose a reason for hiding this comment

jaredlll08 commented May 12, 2023

jaredlll08 commented May 13, 2023

jaredlll08 May 17, 2023

Choose a reason for hiding this comment

Darkhax May 17, 2023

Choose a reason for hiding this comment

jaredlll08 commented Nov 11, 2023

ghost commented May 12, 2023 •

edited by ghost

Loading

jaredlll08 left a comment •

edited

Loading

jaredlll08 May 12, 2023 •

edited

Loading