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

QOL: Making use of Forgix #384

Merged
merged 2 commits into from
Nov 24, 2024

Conversation

Redstoneguy129
Copy link
Contributor

It is commendable that Architectury is being utilized, allowing compatibility with multiple mod loaders. However, this approach may lead to confusion among less technically adept players. Providing a single jar file for distribution would alleviate any potential misunderstandings. While this solution may not directly enhance the mod's functionality, it would significantly simplify the installation process.

@Duzos
Copy link
Contributor

Duzos commented Oct 22, 2024

looks cool and sounds good

@Jeryn99
Copy link
Collaborator

Jeryn99 commented Oct 22, 2024

I would need to see some evidence that forgix is actually stable and handles everything as it should, there was a attempt at this when forgix was created but it always ended up with errors

@Redstoneguy129
Copy link
Contributor Author

I would need to see some evidence that forgix is actually stable and handles everything as it should, there was a attempt at this when forgix was created but it always ended up with errors

Forgix has changed quite a bit since it first released. Works in all the projects i've developed in the past and tested when i implemented it here.
It simply just adds both the forge, common and fabric packages to the same build instead of being modloader specific. Architectury already tells where the mod loader specific classes/methods are so there aren't conflicts.

@Jeryn99
Copy link
Collaborator

Jeryn99 commented Oct 22, 2024

Although I understand its functionality and mechanism, I am uncertain whether it is essential. We already incorporate the mod loader within the file name and display name on the platforms, rendering a combined jar redundant.

Furthermore, will this function seamlessly with UnifiedPublishing or the Jar-in-Jar approach we employ for Fabric?

@Redstoneguy129
Copy link
Contributor Author

Although I understand its functionality and mechanism, I am uncertain whether it is essential. We already incorporate the mod loader within the file name and display name on the platforms, rendering a combined jar redundant.

Furthermore, will this function seamlessly with UnifiedPublishing or the Jar-in-Jar approach we employ for Fabric?

I don't quite get what you mean by UnifiedPublishing. If it's referring to uploading to Modrinth/Curseforge then that works fine as they allow you to select multiple mod-loader tags and so apps like Curseforge and prism will work fine with installing the mod.
Jar-in-Jar i personally know works seamlessly, I've implemented jar in jar for mod-loader specific scenarios in a few projects and it came out fine.

And yes, this PR is not essential to improving the functionality of the mod. But the user experience when manually installing, not everyone looks at the mod loader tags surprisingly.

@Jeryn99
Copy link
Collaborator

Jeryn99 commented Oct 22, 2024

I would consider merging this if you can validate that it functions correctly with unified publishing.

For more information, please refer to the following link: https://github.com/shedaniel/unified-publishing

@Redstoneguy129
Copy link
Contributor Author

I would consider merging this if you can validate that it functions correctly with unified publishing.

For more information, please refer to the following link: https://github.com/shedaniel/unified-publishing

https://github.com/shedaniel/unified-publishing/blob/193c95ef3a5edbe7f336d1f05fed4973e0bf36be/src/main/java/me/shedaniel/unifiedpublishing/CurseforgePublishingTarget.java#L75
https://github.com/shedaniel/unified-publishing/blob/193c95ef3a5edbe7f336d1f05fed4973e0bf36be/src/main/java/me/shedaniel/unifiedpublishing/impl/TaskModrinthUpload.java#L186

These lines show it is a feature to add multiple mod-loaders to a single release. If you approve I can test by creating a Curseforge project, uploading a release then deleting after the test?

@Redstoneguy129
Copy link
Contributor Author

Actually i think i see what you mean. UnifiedPublishing interacts with the task directly? I'll do some personal tests and get back on the results.

@Redstoneguy129
Copy link
Contributor Author

Solved. In the past it wouldn't work as UnifiedPublishing required the tasks to be given directly. However you can now give it a file. So you instead dependsOn the mergeJars before the publishing. Will be pushing the updated build.gradle soon

@Redstoneguy129
Copy link
Contributor Author

Redstoneguy129 commented Oct 23, 2024

image
This image shows that the jar was generated and released with the correct tags.

tasks.mergeJars.dependsOn(tasks.build)

tasks.register('publishMeEverywhere') {
    dependsOn tasks.mergeJars, tasks.publishUnified
}

tasks.publishUnified {
    mustRunAfter tasks.mergeJars
}

mainPublication project.layout.file(project.provider {
            new File("${project.projectDir}/build/libs/$rootProject.jar_name-mc$project.minecraft_version-v${rootProject.mod_version}.jar")
        })

^ Changes that had to be made to accommodate Forgix

@Jeryn99
Copy link
Collaborator

Jeryn99 commented Nov 24, 2024

Cool, I will merge this - please PR this to the other 1.20 branch aswell

@Jeryn99 Jeryn99 merged commit bca1290 into WhoCraft:minecraft/1.20 Nov 24, 2024
@Jeryn99
Copy link
Collaborator

Jeryn99 commented Nov 28, 2024

I merged this and it worked great! Until used with any mod that depended on Tardis Refined
Forgix appears to sort the codebase into a forge folder and a fabric folder and that's great!
But not so great when it also changes the package of a class...

Jeryn99 added a commit that referenced this pull request Nov 28, 2024
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.

3 participants