Hook BlockState's Json loading to add support for simplified Forge format. #1885

Merged
merged 1 commit into from Jun 2, 2015

Projects

None yet

10 participants

@LexManos
Member

Due to the sever amount of whining. And nobody else stepping up to try and handle this. I've taken a crack at it.
This is the first pass at re-working the model formats that Minecraft provides.
This is done after a non-insignificant amount of discussion with Mojangsters, about the possible changes coming to future versions of their system. The basic concept is close to what I understand they want to go forward with so ya.

What is the point of this?
To once and for all make the 'OMFG THE SKY IS FALLING' people shut up.
So lets get to it this is what it does.
It allows you to use a less-verbose blockstates.json file.
What before would be represented by this: https://github.com/Deadrik/TFC2/blob/master/src/Resources/assets/tfc2/blockstates/grass.json and https://github.com/Deadrik/TFC2/tree/master/src/Resources/assets/tfc2/models/block/Grass
Is now represented by this: https://gist.github.com/LexManos/e9d58bd55b2c8fc0c8b1

And an example of what it looks like in game:
http://puu.sh/hTYHW/08c68329ec.png
http://puu.sh/hTYJy/2cc1c6cf38.png

I have NOT even bothered looking at the Item models yet. This is meant purely for blockstates. I may expand to items at a later point.
This is a FIRST DRAFT.
It needs a lot of idiot proofing, most notably something to tell people that this ONLY WORKS WHEN USING VANILLA MODELS.

This thread is not for arguing the fundamental 'should it be done' it's for addressing the idiot-proofing and functionality side. If someone can think of how to make it function for other models easily that'd be awesome.

@grum
grum commented May 20, 2015

/me votes if someone is going to whip up a custom format to go with the format we'll very likely (99% sure) will be using ourselves in the future (1.9).

https://gist.github.com/grum/91331002c6149e39c4c0

@me4502
Contributor
me4502 commented May 20, 2015

I agree with @grum

@spaceemotion

A question about the capabilities of block models: Was the ability to created slanted blocks been removed? Or is it still possible to create custom models via code (later on through Forge, etc.)?

@chibill
chibill commented May 20, 2015

Slants are possible right now with normal block models.

@spaceemotion

@chibill Ah thanks.

I was asking because it wasn't quite clear when looking at the JSON. From what I understand the "from" and "to" are just vectors indicating the boundaries of each element. How would one add the 8 vectors required for a slanted block?

@LexManos
Member

There is no way that we will ever be able to create something that is 100% Forwards compatible. But this is SIMILAR and allows a lot of flexibility. Whenever Mojang comes out with a stable version we can adapt.

@me4502
Contributor
me4502 commented May 21, 2015

Honestly, semi automated conversion would be more useful than having a set format. At least that way modders have least possible work to do.

@Aroma1997

I agree with me4502 there, it would be more useful to have these models automatically generated and only having to adjust them than have such a thing. But still, I think that simplified format is better than the current one.

@me4502
Contributor
me4502 commented May 23, 2015

All we really need is a program that can interpret all the different formats, and then save them. That way it can load and save any model format, allowing conversion.

@LexManos
Member

Feel free to make that program, Forge has an easily extendable model framework you can add whatever format you want to.
As for being convertible to Vanilla format, once Vanilla comes out with something useful we'll adapt that's how Forge always has been.
Also, Zaggy is working on sub-modules https://github.com/LexManos/MinecraftForge/pull/1
Using submodules causes the json to be a bit more verbose, and then Zaggy likes liberal use of new lines. But should provide the functionality that you guys want. His example is a fence using two models, a center pillar and side. Should be good to go, just need to sit down and test his PR.

@RainWarrior RainWarrior commented on an outdated diff May 28, 2015
...net/minecraftforge/client/model/BlockStateLoader.java
+ private List<Variant> getSubmodelPermutations(Variant baseVar, Map<String, List<Version1.Variant>> variants)
+ {
+ List<String> sorted = Lists.newArrayList(variants.keySet());
+ Collections.sort(sorted); // Sort to get consistent results.
+ return getSubmodelPermutations(baseVar, sorted, variants, 0, new HashMap<String, Variant>(), new ArrayList<Variant>());
+ }
+ }
+
+ public static class Variant
+ {
+ public static final Object SET_VALUE = new Object();
+
+ // ******Set are used to determine which values must be changed to inherit values from a parent.
+ public ResourceLocation model = null;
+ public boolean modelSet = false;
+ public ModelRotation rotation = null;
@RainWarrior
RainWarrior May 28, 2015 Member

Would be very nice to be able to support deserealization of custom rotations - ModelRotation is quite limited. Making it wotk with vanilla models is quite a task though.
Also, I'd probably use Optional for most of these fields instead of separate marker bools, since performance isn't critical, but it's also not a critical style choice :P

@RainWarrior RainWarrior commented on the diff May 28, 2015
...java/net/minecraftforge/client/model/ModelLoader.java
@@ -336,7 +417,15 @@ public WeightedRandomModel(Variants variants)
{
ResourceLocation loc = v.getModelLocation();
locations.add(loc);
- IModel model = new WeightedPartWrapper(getModel(loc));
+
+ IModel model = getModel(loc);
+ if (v instanceof ISmartVariant)
+ {
+ model = ((ISmartVariant)v).process(model, ModelLoader.this);
@RainWarrior
RainWarrior May 28, 2015 Member

I don't see any way of referencing these variant-applied models in code/in json, yet I think it should be useful. Maybe a way of constructing and resolving resource locations referring to the blockstate definitions directly (and adding a possibility to recursively apply blockstate modifications) is a good idea.

@RainWarrior RainWarrior commented on an outdated diff May 28, 2015
.../java/net/minecraftforge/client/model/MultiModel.java
+ @Override
+ public boolean isBuiltInRenderer()
+ {
+ return base.isBuiltInRenderer();
+ }
+
+ @Override
+ public TextureAtlasSprite getTexture()
+ {
+ return base.getTexture();
+ }
+
+ @Override
+ public ItemCameraTransforms getItemCameraTransforms()
+ {
+ return base.getItemCameraTransforms();
@RainWarrior
RainWarrior May 28, 2015 Member

transforming the subparts individually is something that may come up in the future. Or not :P

@RainWarrior RainWarrior commented on an outdated diff May 28, 2015
.../java/net/minecraftforge/client/model/MultiModel.java
+import net.minecraft.client.renderer.block.model.BakedQuad;
+import net.minecraft.client.renderer.block.model.ItemCameraTransforms;
+import net.minecraft.client.renderer.texture.TextureAtlasSprite;
+import net.minecraft.client.renderer.vertex.VertexFormat;
+import net.minecraft.util.EnumFacing;
+import net.minecraft.util.ResourceLocation;
+import net.minecraftforge.client.model.BlockStateLoader.Version1;
+
+import com.google.common.base.Function;
+
+public class MultiModel implements IModel
+{
+ public static class Baked implements IFlexibleBakedModel
+ {
+ protected IFlexibleBakedModel base;
+ protected IFlexibleBakedModel[] parts;
@RainWarrior
RainWarrior May 28, 2015 Member

maybe ImmutableList is better?

@RainWarrior RainWarrior commented on an outdated diff May 28, 2015
.../java/net/minecraftforge/client/model/MultiModel.java
+ @Override
+ public TextureAtlasSprite getTexture()
+ {
+ return base.getTexture();
+ }
+
+ @Override
+ public ItemCameraTransforms getItemCameraTransforms()
+ {
+ return base.getItemCameraTransforms();
+ }
+
+ @Override
+ public List<BakedQuad> getFaceQuads(EnumFacing side)
+ {
+ ArrayList<BakedQuad> ret = new ArrayList(base.getFaceQuads(side));
@RainWarrior
RainWarrior May 28, 2015 Member

This should probably be cached, especially if parts array becomes immutable

@RainWarrior RainWarrior commented on an outdated diff May 28, 2015
.../java/net/minecraftforge/client/model/MultiModel.java
+
+ for (IFlexibleBakedModel bakedPart : parts)
+ ret.addAll(bakedPart.getGeneralQuads());
+
+ return ret;
+ }
+
+ @Override
+ public VertexFormat getFormat()
+ {
+ return base.getFormat();
+ }
+ }
+
+ public IModel base;
+ public Map<IModel, IModelState> parts;
@RainWarrior
RainWarrior May 28, 2015 Member

Same comment as for baked version - making this immutable is probably better. Also, why public?

@RainWarrior RainWarrior and 2 others commented on an outdated diff May 28, 2015
...t/minecraftforge/client/model/IRetexturableModel.java
+ * Applies new textures to the model.
+ * The returned model should be independent of the accessed one,
+ * as a model should be able to be retextured multiple times producing
+ * a separate model each time.
+ *
+ * The input map MAY map to NULL which should be used to indicate the
+ * texture was removed. Handling of that is up to the model itself.
+ * Such as using default, missing texture, or removing vertices.
+ *
+ * The input should be considered a DIFF of the old textures, not a
+ * replacement as it may not contain everything.
+ *
+ * @param textures New
+ * @return Model with textures applied.
+ */
+ IModel retexture(Map<String, String> textures);
@RainWarrior
RainWarrior May 28, 2015 Member

Map<String, ResourceLocation> maybe?

@LexManos
LexManos May 29, 2015 Member

No there should be no reason to ever return multiple models from this.

@RainWarrior
RainWarrior May 29, 2015 Member

Github ate my generic, I meant to say

Map<String, ResourceLocation>
@Zaggy1024
Zaggy1024 May 29, 2015 Contributor

ModelBlock doesn't use a Map<String, ResourceLocation>, so neither do we. :P

@RainWarrior RainWarrior and 2 others commented on an outdated diff May 28, 2015
...java/net/minecraftforge/client/model/ModelLoader.java
+ if (e.getValue() == null)
+ {
+ removed.add(e.getKey());
+ neweModel.textures.remove(e.getKey());
+ }
+ else
+ neweModel.textures.put(e.getKey(), e.getValue());
+ }
+
+ // Map the model's texture references as if it was the parent of a model with the retexture map as its textures.
+ Map<String, String> remapped = Maps.newHashMap();
+ for (Entry<String, String> e : (Set<Entry<String, String>>)neweModel.textures.entrySet())
+ if (e.getValue().startsWith("#"))
+ remapped.put(e.getKey(), e.getValue());
+
+ int passes = 5;
@RainWarrior
RainWarrior May 28, 2015 Member

why 5 passes? What exactly is this code doing?

@Zaggy1024
Zaggy1024 May 28, 2015 Contributor

Like the comment says, it's so that retexture treats the map as the texture map of a child model, so that if a model references "#blah", and "blah" is defined the textures map, it'll resolve that.
Multiple passes might not really be necessary, I suppose.

@LexManos
LexManos May 29, 2015 Member

No, multiple passes shouldn't be needed.

@Zaggy1024
Contributor

I've put in a pull request to address RainWarrior's concerns. https://github.com/LexManos/MinecraftForge/pull/3

@RainWarrior RainWarrior commented on an outdated diff May 31, 2015
.../java/net/minecraftforge/client/model/MultiModel.java
+ return internalBase.getFormat();
+ }
+
+ public IFlexibleBakedModel getBaseModel()
+ {
+ return base;
+ }
+
+ public Map<String, IFlexibleBakedModel> getParts()
+ {
+ return parts;
+ }
+ }
+
+ protected final IModel base;
+ protected final Map<String, Pair<IModel, IModelState>> parts;
@RainWarrior
RainWarrior May 31, 2015 Member

ImmutableMap? :P

@LexManos LexManos merged commit 61f9c5a into MinecraftForge:master Jun 2, 2015
@ShetiPhian ShetiPhian commented on the diff Jun 9, 2015
...et/minecraftforge/client/model/ForgeBlockStateV1.java
+ this.modelSet = other.modelSet;
+ this.rotation = other.rotation;
+ this.uvLock = other.uvLock;
+ this.weight = other.weight;
+ this.textures.putAll(other.textures);
+ this.submodels.putAll(other.submodels);
+ this.simpleSubmodels.putAll(other.simpleSubmodels);
+ this.customData.putAll(other.customData);
+ }
+
+ /**
+ * Sets values in this variant to the input's values only if they haven't been set already. Essentially inherits values from o.
+ */
+ ForgeBlockStateV1.Variant sync(ForgeBlockStateV1.Variant parent)
+ {
+ if (!this.modelSet) this.model = parent.model;
@ShetiPhian
ShetiPhian Jun 9, 2015 Contributor

this.modelSet is never changed, which causes improper model overwrites.
Adding the line bellow after the model is set to the parent's model corrects this.
if (!this.modelSet) this.modelSet = parent.modelSet;

@DanielHuisman

I've been thinking about this new format and it would be useful to have a more programmable definition format. We can extend the current format with variables, for example stone={1},open={2},direction={3}. The loader can parse this and loop over all possible values to generate the corresponding textures, for example: stones/stone_{1}_{2}_{3}. This gist should explain what I mean:
https://gist.github.com/DanielHuisman/b5d8e6e7439ba2ab6090

For vanilla metadata this might not be too useful as there can only be 16 combinations at most, but I'm currently working on a coremod the extends the metadata to a higher maximum (Before anybody complains about how impossible that is, I have a working prototype). Especially when using integer properties that can range from 0-1023 for example, this modification to the loader format would save a lot of repetitive texture definitions.

If you think this is a good idea I can start working on a pull request.

@LexManos
Member

Because of the way things are done, no that is not possible.
What this edit does is rather very simple and I want to keep it that way.
It doesn't know about any of the possible states at all.
Everything it knows is purely defined within the json file itself.
It has no idea what {1} doesnt mean value of "{1}" and it has no way to tell if that's a valid value or not.

We are not here to make the blocksatate.json a scripting language, this was PURELY to deal with the 99% usecase people were complaining about.

@DanielHuisman

Ok, makes sense. I will try to implement a custom model / texture loader in my mod.

@LexManos
Member

Be careful with that you need to make sure that you don't destroy the whole baked model concept. Its how the the rendering in 1.8 gets its power it doesn't have to generate all the models/gl calls every frame.

@DanielHuisman

Ok, I will try to be careful. But pre-generating every model takes up a lot of memory, right?
Especially if you have a lot of blocks or a lot of different metadata.
(Haven't looked too much into the new model system, so I don't know how it's implemented)

@LexManos
Member

Depends on what your definition of a lot is, It also depends on how complex the model is obviously something that is 50,000 quads is gunna take up a bit of memory over something that is like 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment