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

Fix inconsistency between vanilla and modded glass. Closes #4679 #4680

Merged
merged 2 commits into from Feb 2, 2018

Conversation

Managarmr719
Copy link
Contributor

@Managarmr719 Managarmr719 commented Jan 18, 2018

Fixes #4679.

Changed BlockFluidRenderer::renderFluid to check the material instead of the block itself, so all blocks with Material.GLASS should now work fine.

2018-01-18_01 31 13

@Managarmr719 Managarmr719 changed the title Fixed https://github.com/MinecraftForge/MinecraftForge/issues/4679 #4679 Jan 18, 2018
@Managarmr719 Managarmr719 changed the title #4679 Fixed #4679 Jan 18, 2018
@mezz mezz added Feature This request implements a new feature. 1.12 labels Jan 18, 2018
@mezz mezz added the Assigned This request has been assigned to a core developer. Will not go stale. label Jan 18, 2018
@mezz
Copy link
Contributor

mezz commented Jan 18, 2018

Looks like a good, simple fix.

@williewillus
Copy link
Contributor

do forge fluids handle this properly? they use ModelFluid instead of vanilla's BlockFluidRenderer

@mezz
Copy link
Contributor

mezz commented Jan 18, 2018

I just checked and Forge fluids don't handle any kind of glass, the sides are always flowing. I think that can be considered a different issue though, the Forge fluids probably still have a few rendering inconsistencies vs vanilla.

@mezz
Copy link
Contributor

mezz commented Jan 18, 2018

I think this will make water render incorrectly against glass panes.
Please check that the block is solid on the side that the fluid is against.

@mezz mezz removed the Assigned This request has been assigned to a core developer. Will not go stale. label Jan 18, 2018
@HenryLoenwind
Copy link

I don't think glass isSolid?

@Managarmr719
Copy link
Contributor Author

Managarmr719 commented Jan 18, 2018

@mezz The way it is now, water against glass panes renders water_overlay, which differs from vanilla behaviour, normally it would render water_overlay for glass blocks and water_flow for everything else (including glass panes).

@Managarmr719
Copy link
Contributor Author

@mezz It does cause some issues with glass panes. :\

Before:
2018-01-18_16 05 02

After:
2018-01-18_16 03 16

@Managarmr719
Copy link
Contributor Author

Managarmr719 commented Jan 18, 2018

Water blocks against glass panes now render correctly. BlockFluidRenderer::renderFluid now checks if the adjacent blocks are not glass panes and only then renders water_overlay.

@mezz
Copy link
Contributor

mezz commented Jan 19, 2018

I think it's important to look at the reasoning behind why glass works this way, and look at the block's properties instead of using anything like == or instanceof Block. Ideally these rules should work for modded blocks properly, without people having to really think about it.

@Managarmr719
Copy link
Contributor Author

Managarmr719 commented Jan 19, 2018

Something like this if (state.getBlockFaceShape(blockAccess, blockpos, EnumFacing.UP) == BlockFaceShape.SOLID) { ... }? By doing things this way, only "full" blocks should use water_overlay and from what I tested it seems to work fine, even with modded blocks.

@Managarmr719
Copy link
Contributor Author

It would also need to check the bottom otherwise slabs would not work properly.

@Managarmr719
Copy link
Contributor Author

Improved over the example above. Now checks the block faces that are against the water block to see if they're solid.

Copy link
Contributor

@mezz mezz left a comment

Choose a reason for hiding this comment

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

Looks good, just need to fix the patches so there are no import changes.

-import net.minecraft.block.Block;
import net.minecraft.block.BlockLiquid;
import net.minecraft.block.material.Material;
+import net.minecraft.block.state.BlockFaceShape;
Copy link
Contributor

Choose a reason for hiding this comment

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

Patches to Minecraft code should not change the imports.
Instead of importing, use the fully qualifies name where you need it, like net.minecraft.block.state.BlockFaceShape.SOLID

+ IBlockState state = p_178270_1_.func_180495_p(blockpos);

- if (block == Blocks.field_150359_w || block == Blocks.field_150399_cn)
+ if (state.func_193401_d(p_178270_1_, blockpos, EnumFacing.field_82609_l[i1+2].func_176734_d()) == net.minecraft.block.state.BlockFaceShape.SOLID)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think EnumFacing.VALUES[i1+2] should be EnumFacing.getHorizontal(i1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't work because VALUES and HORIZONTALS are ordered differently. (getHorizontal() just returns the values from HORIZONTALS)

VALUES[2] -> NORTH
VALUES[3] -> SOUTH
VALUES[4] -> WEST
VALUES[5] -> EAST

HORIZONTALS[0] -> SOUTH
HORIZONTALS[1] -> WEST
HORIZONTALS[2] -> NORTH
HORIZONTALS[3] -> EAST

BlockFluidRenderer::renderFluid checks the adjacent blocks in the same order as VALUES, so if I used EnumFacing.getHorizontal(i1) it would check the wrong side of the block, it wouldn't check the side that's against water.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably would make sense to make HORIZONTALS follow the same order as VALUES. Then it would be possible to use getHorizontal(i1) instead of VALUES[i1+2]. Although it would break something that depends on it being in the current order. :\

Copy link
Contributor

Choose a reason for hiding this comment

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

Yuck, ok. Nothing we can do about that, yeah.

@mezz
Copy link
Contributor

mezz commented Jan 23, 2018

Can you make a final screenshot to show this off?

@Managarmr719
Copy link
Contributor Author

Managarmr719 commented Jan 23, 2018

Here are some screenshots but I also found some issues.

water2

Here's an image showing what makes water render water_overlay and what doesn't:
2018-01-23_11 45 37

Water next to any solid face now renders water_overlay.

  1. Glass Pane
  2. Diamond Glass Pane - Diamond Glass
  3. Diamond Glass Stairs (side) - Diamond Glass
  4. Stone Slab
  5. Structuralduct with Hardened Silver Glass Cover - Thermal Dynamics *1
  6. Cobblestone Structure Pipe with Facade: Glass - Buildcraft *2
  7. Thickened Glass - Extra Utilities
  8. Glass
  9. Diamond Glass - Diamond Glass
  10. Ice
  11. Hardened Copper Glass - Thermal Foundation
  12. Diamond Glass Stairs (back to water) - Diamond Glass

Issues:

1.

As you can see in the image below, it wasn't working properly, the water is flowing even with the cover. I managed to fix this by also checking isSideSolid().

Before:
2018-01-23_16 15 54

After:
2018-01-23_16 14 02

2.

This one also doesn't work right and it's even worse than the first. It's always solid even without a facade on, wich makes it look like this when underwater:
2018-01-23_17 13 23

@mezz
Copy link
Contributor

mezz commented Jan 24, 2018

I think just one check for getBlockFaceShape should be good enough, checking both seems like a workaround for a minor bug in BuildCraft.

@Managarmr719
Copy link
Contributor Author

Reset to previous commit, I don't know if that's the best way to do it, but I thought it would make more sense than leaving it in and having another undoing it. Anyway, now it only checks getBlockFaceShape.

@mezz mezz added the Assigned This request has been assigned to a core developer. Will not go stale. label Jan 25, 2018
@Managarmr719 Managarmr719 changed the title Fixed #4679 Fix inconsistency between vanilla and modded glass. Closes #4679 Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.12 Assigned This request has been assigned to a core developer. Will not go stale. Feature This request implements a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants