Split conversion process into 3 parts, make converters more standalone#42
Conversation
onebeastchris
left a comment
onebeastchris
left a comment
There was a problem hiding this comment.
LGTM! thanks for working on this :)
| public record ExtractionContext(@Deprecated(forRemoval = true) BedrockResourcePack bedrockResourcePack, | ||
| Optional<ResourcePack> vanillaPack, LogListener logListener) implements LogListenerHelper { |
There was a problem hiding this comment.
a deprecation note here would be neat
There was a problem hiding this comment.
What is the purpose of this deprecation? All these changes are BC breaking so having a deprecation seems pointless
There was a problem hiding this comment.
The deprecation was made because the bedrock resource pack should not be accessible here, as this is the context for the extraction phase. The bedrock pack should only be accessible in the last phase, where all converted bedrock assets are combined into the bedrock pack.
The reason this is still here is because the bedrock pack is written to by the texture transformers, which are executed in the extraction phase at the moment (even though they shouldn't be). I'll leave an explanation with more detail further below in a reply to another comment.
| // TODO: Convert item models but save differently? | ||
| if (value.startsWith("item/")) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This seems like something where it'd be useful to also check for the target mc version as item models are "new"?
There was a problem hiding this comment.
This is talking about the actual item models (in assets/<namespace>/models/item, which have existed for a long time), not the item definition assets in the assets/<namespace>/items directory. I should also note this comment wasn't left by me, Git is a bit weird with the blaming here.
| // TODO: Don't hardcode all this | ||
| Description description = new Description(); | ||
| description.identifier(String.format(GEOMETRY_FORMAT, geoName)); | ||
| description.textureWidth(16); | ||
| description.textureHeight(16); | ||
| description.visibleBoundsWidth(2); | ||
| description.visibleBoundsHeight(2); | ||
| description.visibleBoundsOffset(new float[] { 0.0f, 0.25f, 0.0f }); | ||
| geometry.description(description); |
There was a problem hiding this comment.
Are these something the user would need to provide, or could these be automatically extracted?
There was a problem hiding this comment.
As far as I know these things are the same values Blockbench uses for its Java -> bedrock model conversion, I'm not sure what exactly they affect. I'm also not sure if Blockbench always uses the same values, or if they are dependent on the model. I'd have to look into it.
| // TODO ideally textures should be transformed individually in the convert process, and not together in the extraction process, but this is hard to achieve, | ||
| // TODO and will need another big refactor to the texture transformation code | ||
| // TODO for now this will work, but for library users it might be nice to be able to properly convert singular textures with transformations |
There was a problem hiding this comment.
for the meantime: are those transformations done via utility methods? e.g. might be worth considering to add utils that'd allow creating bedrock textures from Java's
There was a problem hiding this comment.
Couldn't a user of the library just create their own and make sure the ServiceLoader catches it? Therefor adding it to the transformers list
There was a problem hiding this comment.
The main issue the TODO is talking about here is the fact that texture transformations are executed within the extraction phase of the conversion pipeline. On top of this, there are also texture transformers that write to the bedrock pack, which is definitely not something that should happen, given these are executed in the extraction phase.
Ideally, texture transformers would be executed within the conversion phase, as that is essentially what they are doing: transforming/converting a texture from Java's format to bedrock's format. However, there are some texture transformers that take in multiple textures and merge them together, for example the atlas transformer, which takes in multiple Java textures to transform them into a single atlas image for bedrock. This causes some issues when trying to use them in the conversion phase, as that only takes one texture at a time at the moment.
On top of this, the texture transformers are not really built to work this way. TransformContext has poll, peek, and offer methods. Essentially, the way transformers are designed now, all textures are taken from the Java resourcepack, taken through all the transformers (which can poll any of the textures in the resourcepack, and offer a transformed one), and then taken to the mapping process.
Personally, I think it is best to register texture transformers for a "texture group": this limits which textures texture transformers can touch, and allows the extraction phase to simply group all textures in a resourcepack (not-transformed ones would all go in an "untransformed" group). The actual converter can then invoke the transformer of a group, if any exists, and map the resulting textures afterwards, and return them for the combiner to merge into the bedrock pack.
These changes would allow library users to have more control over which textures are transformed and which aren't, along with other benefits, such as having individual textures transformed without having to provide an entire resourcepack. Overall it would follow the design pattern of the conversion pipeline better.
However, these changes would lead to pretty decent refactors to the texture transforming system. I'm fine with including those in this PR, but that would give it a bit of a delay. I could also make them later on, however that'd result in 2 breaking changes shortly after each other.
rtm516
left a comment
rtm516
left a comment
There was a problem hiding this comment.
Mostly nitpics around copyright.
Can we also change the org.geysermc.pack.converter.converter package to be something else, provided we can find something else that makes sense?
| // TODO ideally textures should be transformed individually in the convert process, and not together in the extraction process, but this is hard to achieve, | ||
| // TODO and will need another big refactor to the texture transformation code | ||
| // TODO for now this will work, but for library users it might be nice to be able to properly convert singular textures with transformations |
There was a problem hiding this comment.
Couldn't a user of the library just create their own and make sure the ServiceLoader catches it? Therefor adding it to the transformers list
|
Will fix the copyright changes with the next commit. I'm not sure what a good alternative name would be for the |
…e KeyUtil to lessen code warnings
4 participants
This PR refactors the converters of PackConverter. Essentially, the goal is to remove the "Pack" part of PackConverter: converters will be able to act more standalone from each other, allowing library users to manually convert assets loaded with Creative, instead of having PackConverter convert the entire resourcepack at once.
The conversion process has been turned into a pipeline consisting of 3 subprocesses:
This process is much more modular compared to what converters are now (consisting of just a single function doing all of the above at once). This allows library users to use PackConverter however they see fit. On top of this, this could also make it easier to incorporate multithreading into PackConverter.
In the code, this results in the following changes (summarised):
ConverterandBaseConverterclasses have been removed, and replaced by 3 functional interfaces representing each part of the conversion pipeline:AssetExtractor,AssetConverter, andAssetCombiner.PackConversionContextandConversionDataand its subclasses have been removed. Conversion context/data is now divided between the various process contexts (ExtractionContext,ConversionContext,CombineContext) (only store very generic data) and the dataAssetExtractors provide.PackConverternow takes a list ofConverterPipelines, instead ofConverters. AConverterPipelineis a combination of anAssetExtractor,AssetConverter, andAssetCombiner.PackConverterwill execute all pipelines upon starting pack conversion.ConverterPipelines are registered in theAssetConvertersclass, which also contains utility methods to create 3rd-party pipelines.ActionListenerhas been reworked and expanded with more functionality. It now executes between the various conversion pipeline processes (post extract, post convert, post merge).ActionListeners aren't registered globally atPackConverteranymore, rather, anActionListenercan be attached to aConverterPipeline.BedrockResourcePack#addExtraFilecan now take aJsonElementdirectly, for convenience and to reduce code duplication.Most of this PR is in a finished state (barring any reviews or requested changes), however, texture conversion does not follow the pipeline accordingly. See the TODOs I have left there for more information.
I have tested this PR and PackConverter is functional with it. However, it could still use some more testing. Things I have tested are basic texture, model, language, splash, icon, and manifest conversion.