BuildCraft|Core

BuildCraft|Core

7M Downloads

Unable to rotate facades in a crafting grid

Cisien opened this issue ยท 13 comments

commented
commented

Lumping in facades of render type 31 not appearing in the list (and possibly others?) into this issue/fix.

commented

I'm looking in to this, but i can't find how to assign this issue to myself.

commented

I've added you to the dev team - you've more than demonstrated that you know what you're doing :-)

The issue with the rendering is related to the filter in https://github.com/BuildCraft/BuildCraft/blob/BuildCraft-5.0.x/common/buildcraft/transport/ItemFacade.java#L125. Was made more robusts as we're now trying to create facades from other blocks, but maybe it's too restrictive now.

commented

That's the same conclusion I came to. I'm going to see what I can do to expand this out as broad as possible. My testing environment has a large portion of the mods that have been updated to support 1.7, so that should help a lot in figuring out what whats, and what doesn't.

commented

I wanted to get this out to give folks a chance to comment on the changes.
What i'm doing is:
I'm removing many of the hard-coded restrictions, such as blocks with TEs, or blocks that are not opaque.

A couple of the hard coded checks will remain to prevent breakage in vanilla, and general odd-ness/uncraftable facades:

  • Blocking facades if it's block.renderAsNormalBlock() comes back false (and it's not glass/stained glass)
  • Blocking facades if it's one of the springs, or a pipe.

Adding a facade blacklist to the config, this is populated with most (all?) of the items that were hard-coded previously, in addition to the Buildcraft machines.
The idea here is to not limit the user. If they want grass blocks that aren't colored, they can remove it from the blacklist. The reality is, the bulk of the user base doesn't know that the config even exists, or they are intimidated by it, and leave it stock. This will let the minority who enjoys tweaking or having full control over their environment a place to change defaults.

Adding the blacklist exposed a shortcoming in forge's Configuration class. String array values are not encased in quotes, so characters that are invalid in the schema of the config file, but valid in values (| came up with buildcraft, since it's part of the modid) will cause a parsing exception when loading the config. I worked around this by adding a couple of helper methods to the Buildcraft configuration class which can be used to surround a string in quotes, or to strip the surrounding quotes.

Tossing facades in the crafting will behave slightly differently as well.

  • If the facade's block is a render type of 31 (rotatable block) it will rotate as before.
  • In all other cases, the facade will cycle through the block's metadata. Allowing the user to swap out colors of wool, or stained glass, for example. (I need to validate this actually works with other mods still.

I'll get a PR together for this later today. I want to make sure other mods don't have problems with facades. (Natura/TiCon, AE2 will be good test cases, i'm sure)

commented

If you go this way, let mods tell you what NOT to use as a facade too..

commented

Shouldn't you use block.isOpaque() (or whatever it's called) instead of renderAsNormalBlock?

commented

restricting block.isOpaqueCube() will filter out things that are valid facades such as leaves. Some mods's leaves render with color, so I would like to allow users the option to use this.

renderAsNormalBlock filters out things such as redstone torches.

A change to the above, i'm only initializing blocks that have a render type of 0 or 31. Mods will need to use IMC if they have custom renderers. This is to prevent issues with blocks that don't support block.getIcon(side, meta). (JABBA is an example).

commented

I commented out the IMC code for now, to demonstrate this working with mods, with them not telling us they exist. http://paste.cisien.com/f/fd15edb7.png

commented

I much prefer a whitelist personally.

commented

what if you use a whitelist but if the mod doesn't provide/has one use all possible blocks?

commented

I like your approach @Cisien - thanks for taking ownership of that problem :-) Reviewing the PR.

commented

This approach adds everything that is safe, mod authors can use IMC to "white-list" their blocks that weren't auto detected. Users can pick blocks that don't look/render right off using the blacklist.

If we just give users an empty white list, it will remain empty for the majority of users, sadly, and it's not really practical to maintain the white list ourselves.

I'll think through a white-list type setup allowing users to add facades that weren't auto detected.

3020735