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

Repo-based creator templates #2304

Merged
merged 118 commits into from
Jul 12, 2024

Conversation

RedNesto
Copy link
Member

@RedNesto RedNesto commented Jun 3, 2024

Why?

The main motivation is to offload the templates to a separate repo, which are updated independently when opening the creator for the first time in the IDE session. This means templates can be easily updated and tested in real time, no IDE restart (thank goodness.)

Also the creator file template settings became really cluttered and the UI is quite clunky sometimes. Now the templates are stored in a flat dir (zip archives are also supported.)

This also opens the door for anyone to create their own templates, and makes maintaining our templates so much easier (well, I might be biased on that.)

Oh yeah and it also gets rid of some "magic" code gen that breaks embarrassingly often for obscure reasons, so the fabric creator won't have this nasty issue where the main class is incomplete.

Overview

The templates are still Velocity templates, but they are now put together by a JSON file describing a specific template. See the templates repo at https://github.com/minecraft-dev/templates.

Template descriptors mostly contain properties (most of which are displayed in the creator UI) and the template files.

All file templates have access to all the properties in the descriptor. Some string in the descriptor also have access to properties (file template, destination and condition.)

Properties cannot reference properties declared after them (they are setup one by one in a single step.) I chose this limitation so that we cannot easily cause recursion.

Some properties will be of a custom type, or expose an existing class of the plugin, all of those should be marked with @TemplateApi and we should pay attention to not break the source compatibility of those classes without a good reason.

Used templates are remembered (only the path, not the files contents), there's no way to register them permanently (this could be added to the main settings eventually) Template repositories can be configured in the main configurable, recent templates are no longer remembered.

Fixes #2294 #2264 #2254 #2252 #2141 #2118 #1835 #804 #771

@Earthcomputer
Copy link
Member

I've only spent about 5 minutes looking at it so far, but one immediate question I have, is how are you going to make the options and labels translatable?

@RedNesto
Copy link
Member Author

RedNesto commented Jun 5, 2024

Yeah I've been thinking about it, at least for strings in our resource bundle. Unsure what would be the best way to handle translations provided by the repository itself.

Copy link
Member

@Earthcomputer Earthcomputer left a comment

Choose a reason for hiding this comment

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

Haven't tested it yet, but here are some thoughts having read through it. I'm assuming that you'll get rid of the old creator code once this is done?

.gitmodules Outdated Show resolved Hide resolved
src/main/kotlin/creator/custom/CustomPlatformStep.kt Outdated Show resolved Hide resolved
src/main/kotlin/creator/custom/CustomPlatformStep.kt Outdated Show resolved Hide resolved
data class CreatorJdk(val sdk: Sdk?) {

val javaVersion: Int
get() = sdk?.let { JavaSdk.getInstance().getVersion(it)?.ordinal } ?: 17
Copy link
Member

Choose a reason for hiding this comment

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

Might want to put this 17 in a global default Java version constant somewhere, that we can bump as necessary. MC 1.20.6 is already on Java 21.

Copy link
Member Author

Choose a reason for hiding this comment

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

Might want to put this 17 in a global default Java version constant somewhere

Fair

that we can bump as necessary. MC 1.20.6 is already on Java 21.

Well, this value is supposed to be the default as a fallback. I expect template authors to always provide a default in the descriptor.


override fun serialize(value: Int): String = value.toString()

override fun deserialize(string: String): Int = string.toIntOrNull() ?: 0
Copy link
Member

Choose a reason for hiding this comment

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

Maybe these deserialize functions could return a nullable value, so that if the jsons are invalid a warning could be logged? (and then createDefaultValue could be used)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of the places I couldn't really figure out properly as it was written very early.
Your solution sounds good

Copy link
Member Author

Choose a reason for hiding this comment

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

So I think I'll keep it like that because createDefaultValue can take anything, usually it's a String, but it can be an Int, Boolean or even a Map if the property supports it.

The serialization methods are meant only for remembering values and have to be from/to Strings.

Deserialize can always fallback to the default value, I leave it up to the implementation to decide.

override fun serialize(value: LicenseData): String = value.id

override fun deserialize(string: String): LicenseData =
LicenseData(string, License.byId(string)?.toString() ?: string, ZonedDateTime.now().year.toString())
Copy link
Member

Choose a reason for hiding this comment

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

Maybe licenses could be made extensible, doesn't have to be in this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I simply moved the licenses to the templates repo and reused existing code.
The ids and corresponding files could be provided in a shared descriptor that other descriptors inherit from.

@Earthcomputer
Copy link
Member

I also noticed that in the template repository, Fabric mod ids are validated to a minimum length of 1, whereas on Fabric mod ids have to be at least of length 2.

@RedNesto
Copy link
Member Author

RedNesto commented Jun 6, 2024

I'll look into improving the template selection, maybe split plugins and mods, and move the providers to the settings so we can have repos saved there and displayed in the creator.

@RedNesto RedNesto force-pushed the repo-based-project-templates branch from 6007822 to 973540e Compare June 14, 2024 12:36
@RedNesto RedNesto force-pushed the repo-based-project-templates branch from 50d0d5a to ac7dcfa Compare July 2, 2024 16:03
@RedNesto RedNesto force-pushed the repo-based-project-templates branch from ac7dcfa to 1c64ab4 Compare July 7, 2024 16:06
@Earthcomputer Earthcomputer self-requested a review July 9, 2024 22:35
@terminalsin
Copy link

This is a huge undertaking and I'm so grateful for the people contributing to making this happen. I'm looking forward to this PR being merged :)

@Earthcomputer
Copy link
Member

Hi, I've just tried this out, but it's saying there's no templates available:
image
Do you have any idea why this might be?

@RedNesto
Copy link
Member Author

@Earthcomputer Oh that's not supposed to be like that by default. In the settings page there should be a table of template repositories with a Built In default repo. I'll look into it.

@Earthcomputer
Copy link
Member

Ah yes. It seems the template repositories are empty by default.

@RedNesto
Copy link
Member Author

Should be fixed now

@Earthcomputer
Copy link
Member

Just a couple of things:

  • Use Mixins should be enabled by default on the Fabric and Architectury platforms, and disabled by default on Forge and NeoForge. I've noticed that the value of this checkbox is connected on all platforms, maybe it should be disconnected (stored with a different key).
  • FabricApi and ArchitecturyApi should not be in pascal case.
  • While fetching versions from the internet, there could be an AsyncProgressIcon. In the old creator this is done inside AbstractLatentStep.

@RedNesto
Copy link
Member Author

Fixed all of these.

Also I forgot to answer to your question about the old wizard.
I plan on keeping it until the end of the year, so people who have issues with this new one have a "backup". Guess I should add a warning that it will be removed at some point in the future in the UI.

@Earthcomputer Earthcomputer merged commit 85e493a into minecraft-dev:dev Jul 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Split Source in Fabric
3 participants