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

Added model bake event, ISmartBlock/itemModel, Block.getExtendedState, IExtendedState and IUnlistedProperty #1518

Merged
merged 1 commit into from Dec 12, 2014

Conversation

RainWarrior
Copy link
Member

ModelBakeEvent allows mods to insert custom baked models, much like TextureStitchEvent allows to insert custom textures.
ISmartBlock/ItemModel allows model to get the block/item state for rendering.
IExtendedState allows to use properties that shouldn't be enumerated (IUnlistedProperty).
Example implements this: http://imgur.com/a/FyyJX
example screenshot

@RainWarrior RainWarrior force-pushed the model-event branch 2 times, most recently from cbec820 to e3a219c Compare November 27, 2014 08:34
@hanetzer
Copy link
Contributor

@RainWarrior IIRC, they only accept PRs if they are 1 commit.

@RainWarrior RainWarrior force-pushed the model-event branch 5 times, most recently from 0e2837c to 963ae8b Compare November 28, 2014 20:44
@RainWarrior RainWarrior changed the title Added model bake event. Allows mods to insert custom baked models, much ... Added model bake event and ISmartModel Nov 28, 2014
@RainWarrior RainWarrior force-pushed the model-event branch 5 times, most recently from f855b14 to 3cbc4b2 Compare November 30, 2014 10:02
@RainWarrior RainWarrior changed the title Added model bake event and ISmartModel Added model bake event, ISmartModel and Block.getDynamicState Nov 30, 2014
@RainWarrior RainWarrior force-pushed the model-event branch 2 times, most recently from 555204b to e20dff2 Compare November 30, 2014 15:20
@RainWarrior RainWarrior changed the title Added model bake event, ISmartModel and Block.getDynamicState Added model bake event, ISmartModel, Block.getDynamicState and IDynamicState Nov 30, 2014
@RainWarrior RainWarrior force-pushed the model-event branch 7 times, most recently from 8fac288 to 2c0408d Compare December 3, 2014 17:14
@RainWarrior RainWarrior changed the title Added model bake event, ISmartModel, Block.getDynamicState and IDynamicState Added model bake event, ISmartBlock/itemModel, Block.getExtendedState, IExtendedState and IUnlistedProperty Dec 3, 2014
@RainWarrior RainWarrior force-pushed the model-event branch 4 times, most recently from 0242229 to b4b4fdc Compare December 4, 2014 10:05
@RainWarrior RainWarrior force-pushed the model-event branch 3 times, most recently from 8837ea9 to 613a6c8 Compare December 4, 2014 11:09
…ch like TextureStitchEvent allows to load custom textures), ISmartBlock/ItemModel (ability form models to react to block/item states), Block.getExtendedState, support for unlisted properties in block states. Includes example implementation of http://imgur.com/a/FyyJX
@wgaylord
Copy link

wgaylord commented Dec 4, 2014

I say this is good! I think it we definitly help the comunity.

@LexManos
Copy link
Member

LexManos commented Dec 7, 2014

Needing more modders feedback on this.
If nothing is received i'll pull this in a few days.

import com.google.common.primitives.Ints;

@Mod(modid = ModelBakeEventDebug.MODID, version = ModelBakeEventDebug.VERSION)
public class ModelBakeEventDebug

Choose a reason for hiding this comment

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

Are you sure it's required?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? Every import is necessary, if that's what this is about

@heldplayer
Copy link
Contributor

How will this work with stuff that has multiple or unlimited different kinds of combinations of parts like the MicroBlocks mod?

If a model has to be made for each possible combination the list of models would become huge.

@RainWarrior
Copy link
Member Author

@heldplayer did you look at the included example/description? Do you have any concrete questions?

@heldplayer
Copy link
Contributor

@RainWarrior Yes, but I can't make anything out of it and it doesn't look easy at all.

@LexManos
Copy link
Member

The problem is his examples include everything even a custom model format. And it has no documentation it it.
However yes this lets those who know what they are doing to do the things you're concerned about EASILY.

@asiekierka
Copy link
Contributor

This isn't bad at all! However, I feel that the "IUnlistedProperty" name feels like a bit of a hack: why not something more akin to IRenderingProperty or IModelProperty? I also feel that adding a second, disconnected set of priorities to a BlockState is a bit of a hack.

The limitation to BakedQuads might hurt a few developers, but I need feedback from those who would actually care about this.

Also, we need benchmarks to see whether spamming a hundred BakedQuads doesn't create huge issues with object generation and garbage collection spam, similar to the Vec3Pool removal issues we experience in 1.7.10.

EDIT: Yes, I can work with this for BuildCraft. I'll hate every bloody second of it, but I can work with this if that's what it takes.

@CaptainSly
Copy link

I say keep it. It'll be a great addition. Just listen to @asiekierka though.

@asiekierka
Copy link
Contributor

About @heldplayer's worry with microblocks... An UnlistedProperty can pass any Object through it. So, yes.

@LexManos
Copy link
Member

There is no 'keep it' this is what is going in. If you modders have some valid concerns you can voice them and we can address them before pulling it in officially. Waiting on feedback is the exact reason why I haven't pulled it yet.

As for the 'spamming a hundred BakedQuads' If you make your model correctly there won't be any spam. There will simply be one comprehensive set, and your smart model will select a subset of those to render. Not to hard once you think about it.

As for the name, as this is intended to not explicitly be tied to models/rendering, we didn't want that in the name. This is the best we could come up with that didn't sound stupid and wasn't super long.

@Parker8283
Copy link
Contributor

GitHub for mobile is being dumb and not letting me make a line comment, so I'll just say it here:

Minor detail, but I think the handleItemState method should be renamed to handleItem. It makes sense for handleBlockState, as a block state object is being passed it, but for items, it's an itemstack, and not really a "state". Obviously, this is a very minor detail and just my preference on the name of a method, which in the end does no functional changes. Other than that, PR looks good!

LexManos added a commit that referenced this pull request Dec 12, 2014
Added model bake event, ISmartBlock/itemModel, Block.getExtendedState, IExtendedState and IUnlistedProperty
@LexManos LexManos merged commit ca2d49b into MinecraftForge:1.8 Dec 12, 2014
@dmillerw
Copy link

Something didn't take properly, or get tested.

net/minecraftforge/common/property/ExtendedBlockState.java:21: error: StateImplementation has protected access in BlockState
import net.minecraft.block.state.BlockState.StateImplementation;

@RainWarrior RainWarrior mentioned this pull request Dec 12, 2014
@RainWarrior
Copy link
Member Author

@dmillerw gradle caches need to be cleaned after at/exc change
(just tested with clean install, it works)
(MinecraftForge/ForgeGradle#171)

@RainWarrior
Copy link
Member Author

@dmillerw If you were talking about userdev, then it should be fixed in the next build: d8cb289

@LexManos
Copy link
Member

Ya, seems @RainWarrior Didn't test the userdev setup :p
d8cb289 Fixes it.

@SubscribeEvent
public void onModelBakeEvent(ModelBakeEvent event)
{
TextureAtlasSprite base = Minecraft.getMinecraft().getTextureMapBlocks().getAtlasSprite("minecraft:blocks/slime");
Copy link

Choose a reason for hiding this comment

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

How to get a non vanilla textures to load here? Been reading vanilla source code for about an hour but I don't see no direct way to add textures to the texture atlas

Copy link
Member Author

Choose a reason for hiding this comment

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

1.7 way of TextureStitchEvent.Pre still works

Copy link

Choose a reason for hiding this comment

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

Oh ok, thanks!

@tyronx
Copy link

tyronx commented Dec 30, 2014

There seems to be a lighting issue, the base texture renders with more brightness than normal - that is also visible in the screenshot here (slime block is much brighter)

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.

None yet