BetterPortals

BetterPortals

1M Downloads

Little Tiles Issues

SheltieChan opened this issue · 26 comments

commented

For some reason when this mod and Little Tiles are both installed, Lighting updates cause the LittleTiles blocks to dissapear and re-appear randomly.

After doing some testing with only this mod, Little Tiles, and Optifine (With the Specified Options Selected), Better Portals is the cause.

Mods Used -
1.12.2-forge1.12.2-14.23.5.2836
CreativeCore_v1.9.68_mc1.12.2
LittleTiles_v1.5.0-pre168_mc1.12.2
littleopener-1.0.1
Forgelin-1.8.4
betterportals-0.3.7
OptiFine_1.12.2_HD_U_E3
preview_OptiFine_1.12.2_HD_U_F4_pre3

Example Footage -
https://youtu.be/zYzLkenTR2s
https://youtu.be/sXQXQKqYLH0

commented

This looks like an issue which would need to be fixed in LittleTiles. I'd suggest you report it over there as well.

And just to verify: This happens with only Forge + BP (+ Forgelin) + LT (+ CreativeCore) as well, so no Optifine required, correct? (cause that kind of bug could definitely be caused by OF as well)

From quickly skimming over some of LT's code, this part might be relevant:
https://github.com/CreativeMD/LittleTiles/blob/d12e317f99aa5423768967a16dd2db73cf5b4d53/src/main/java/com/creativemd/littletiles/client/render/cache/RenderingThread.java#L162
Accessing mc.world from another thread in vanilla is sketchy at best (chance of getting the wrong instance is fairly low but generally non-zero), with BP it's almost certainly broken (BP swaps between different mc.worlds multiple times per frame).

commented

Hey, actually I wanted to comment here before you :D

LT generates the render cache in another thread and adds it to the chunk vertex buffer just before it's uploaded (source). I'm pretty sure something gets messed up there.

So it would be good to know what BetterPortals changes about that.

commented

BP re-implements the ChunkRenderDispatcher (source) to support multiple worlds (formerly called views).
It cannot just be swapped out as needed like other parts (e.g. RenderGlobal) because it allocates a fixed number of worker threads which it doesn't share with other instances and tries not to use too much rendering time (and twice not too much time is far too much time).

The only notable difference would be that during uploadChunk, MC may not be in the state which the chunk represents. That's because chunk uploads are only run once, during the primary render pass of each frame and switching to the correct state from there is both difficult and unnecessary.

The only thing in LT's uploadChunk method affected by this is the call to mc.getRenderViewEntity() to sort for transparency. Nothing I'd worry about since transparency sorting is an unsolved problem in BP in general.

Another thing which BP fundamentally changes is the ViewFrustum (source) to allow rendering of possibly distant and/or overlapping views of the same world. The vanilla one has a fixed size array of RenderChunks around the player, BP instead maintains a map of all render chunks and creates arrays for various player positions on demand.
In particular that also means that the active RenderChunk array is always the one from the last call to updateChunkPositions.
If this is called from outside the rendering code (or before updateChunkPositions is called), then it'll potentially return the wrong chunk. Not sure if that's actually the issue since LT seems to be storing the RenderChunk in the tile entity after it has first fetched it (so it shouldn't flicker, should it?)
I'm not sure what the best way of fixing this would be since MC relies on getRenderChunk to wrap around and only return results from within the render distance around the player (iirc).

@ShetlandSan Does this happen only when there's a portal around or does it even happen in a newly generated flat world?

commented

It happens in newly generated flat worlds, as well as worlds that I hadn't even generated a Nether Portal in yet.

Sorry for the late response.

commented

@ShetlandSan

And just to verify: This happens with only Forge + BP (+ Forgelin) + LT (+ CreativeCore) as well, so no Optifine required, correct? (cause that kind of bug could definitely be caused by OF as well)

commented

@CreativeMD No because that's never really what it did in the first place.
getRenderChunk returns a RenderChunk from within view distance around the player. In particular it even returns ones within the view distance even if you pass it coordinates outside the view distance. So depending on which view was rendered last (see my previous post), you're potentially not getting the RenderChunk you were hoping for.

So, while that is an issue in general, it's probably not this issue since this issue happens even when there are no portals around (and with no portals around, i.e. only a single render pass, it should behave just like vanilla).

commented

@Johni0702 LT uses ViewFrustum.getRenderChunk(BlockPos) to get the render chunk at the given block position. Does this still work with your mod?

commented

So i've been doing some checking, and it seems that having Optifine installed causes BetterPortals and Little Tiles to get angry at each other.

Little Tiles and Optifine on their own have no issues, and BetterPortals and Optifine (When I put in the settings you outline in the CurseForge page) have no issues either.

It's only when the three of them try to mix that we see this issue.

commented

Unsure if this helps, But here's some footage of me trying to get the bug to happen from every angle I can make it happen at.

https://youtu.be/iDKKlB3-LV0

commented

@ShetlandSan I'm unable to reproduce this with OF F4_pre3, BP 0.3.7, LT 1.5.0-pre168 and CreativeCore 1.9.68. Not even after resetting all video settings and/or with Dynamic Lighting set to Fancy.

commented

I could not reproduce it either. @ShetlandSan which Optifine version are you using (there are two versions in your first post)?

commented

preview_OptiFine_1.12.2_HD_U_F4_pre3 is the version I have installed.

commented

Can you send me your world, so I can test it myself?

commented

https://www.dropbox.com/s/yoh5lzb5wyyw470/Goo.zip?dl=1

@ShetlandSan send me the world and I could reproduce this issue. It feels like there are plenty of chunk updates and sometimes stuff from LT is not uploaded to the buffer. So the problem must be somewhere here. There multiple options where it could go wrong:

  • the uploadChunk method is not called for some reason (maybe a different chunk render dispatcher is in us) <-- seems unlikely
  • the same buffer is used upload the chunk render data. LT marks each buffer with an additional field added. If the same buffer is used again, it will be skipped.

I don't know what BetterPortals changes about it, but there no issues without.

commented

Hmm... I've updated all mods to exactly the versions you stated (including CMDCam, just in case) but still no luck. My Forge is version 2847 as recommended by OF.

The only thing I do notice is that my FPS take a significant dip (solid 60 → ~30±15) when I move around with Fancy Dynamic Lights and a torch in my hand. I know my system to be slow at uploading data to its GPU, so that would make sense. If the issue is indeed some kind of race condition, that might also be the reason why I'm consistently not experiencing it (and why it's so much more consistent in OP's second video than in yours).

Oh, I've just noticed that MC runs all chunk updates on the main thread for me (I've only passed through two cores to my gaming vm), that would certainly also explain my inability to reproduce it if it's indeed a race condition. I'll see if I can force it to somehow run with background threads and report back if that changes anything.

commented

Indeed, after forcing MC via debugger to run two chunk worker threads, I'm now able to easily reproduce the issue.

Edit: Also happens with Dynamic Lights: Fast
Edit: Even happens with Dynamic Lights: OFF when placing torches nearby
Edit: Also without CMDCam (no surprise) and also without OF
Edit: Also happens if I just place/remove bits anywhere else in the chunk. Doesn't happen when placing/removing normal blocks though.

commented

Interesting ... to be honest I have no idea how this can happen.

Is there any case a chunk could be uploaded without calling the vanilla method (source)? Otherwise it must be the parameters.

commented

@CreativeMD OK, I think I've got a pretty good basic understanding of what's going on.

It basically boils down to the fact LittleChunkDispatcher.uploadChunk assumes to be called right from the ChunkRenderWorker thread which built that particular RenderChunk. LCD.uC then sets the added flag and thereby ignores that particular chunk when it's later called again on the main thread where MC uploads the buffers.
In the process of building a chunk, LT collects all the tile entities in that particular chunk in RenderChunk.tileEntities (via onDoneRendering), then uses it in LCD.uC and subsequently discards it because it no longer needs it after it's done building (this will become important later). Note how this all happens on one ChunkRenderWorker thread.

Now, the important difference with BP is that BP does all the worker thread scheduling by itself (note the else-case in the source you posted), so the vanilla ChunkRenderDispatcher.uploadChunk is only called once the task has gone through that scheduler and has arrived on the main thread.

A race condition appears when the same chunk is twice in quick succession:

  1. The first compile task is dispatched, the ChunkRenderWorker collects LT tile entities in RenderChunk.tileEntities, then hands the task off to BP's scheduler for uploading which might take a bit (who knows what the main thread is currently up to).
  2. While the first task waits, the second is processed (maybe even by the same worker thread), the worker also collects LT tile entities and stores the result in RenderChunk.tileEntities (likely identical result to the previous one), it then hands the task off to BP's scheduler for uploading.
  3. The main thread finally arrives and BP calls the vanilla uploadChunk for the first task. This in turn calls LCD.uC which adds the geometry for all RenderChunk.tileEntities to the buffer and then clears that list and finally MC uploads the buffers as usual. So far, so normal.
  4. BP calls the vanilla uploadChunk for the second task. LCD.uC finds an empty RenderChunk.tileEntities, adds no TEs to the buffer finally MC uploads the buffers as usual and we've got the result we were looking for: No LT TEs are visible in that chunk.

Now, how do we best go about fixing this? As I see it the options are either:

  • Add an explicit call to LCD.uC in BP non-main-thread code path in ViewChunkRenderDispatcher.uploadChunk
  • Change LT's transformer to instead call LCD.uC from ChunkRenderWorker.processTask (where vanilla usually calls uploadChunk from)

IMO the second option is the more idiomatic one because it is where LCD.uC implicitly expects to be called from.

commented

@CreativeMD Did you install any additional mods? As is, with the mods listed in my previous post (except BP 0.3.7.1), I'm still unable to reproduce the issue.
While the initial flickering blocks in the first video are missing the textures for me (cause I don't have that mod installed), they're persistent and don't flicker, regardless of Dynamic Lights setting.

commented

My mod list:

  • CreativeCore_v1.9.70_mc1.12.2
  • LittleTiles_v1.5.0-pre170_mc1.12.2
  • CMDCam_v1.4.4_mc1.12.2 <-- don't think that one does change anything
  • Forgelin-1.8.4
  • betterportals-0.3.7.1
  • OptiFine_1.12.2_HD_U_F4

I couldn't reproduce it either, until I started to shake my mouse and jump around. It eventually started to flicker. It would either remain visible or invisible once I stopped moving my mouse. A block update fixed it again (or moving the mouse heavily).
Also make sure to have dynamic lights on fancy and hold a torch in hand. I will try to record a video of it.

commented

Oh wow, interesting. Makes a lot of sense. Some time ago I left the list untouched, but I was concerned about memory leaks (the tileentities caused that the world wasn't garbage collected in the main menu).

I could use a queue which stores a lists of tileentities, instead of a single list. So multiple updates of the same chunks wouldn't be a problem at all (or an integer which will be incremented every time).

You did some very good research! I will implement a solution for it soon and get back to you. Thank you very much for all your work and effort!

commented

Should I mark this issue as closed since it seems we've figured it out?

commented

I fixed it, new version should be available in a few minutes. Thank you both very much for your help! Good to see it being fixed.

commented

Closing as fixed as of LittleTiles 1.5.0-pre176.

commented

@ShetlandSan I prefer to keep issues open until they're confirmed fixed (and for third-party mods also released).
Unless it needs solving in a different mod and issues were opened on both of them right from the beginning.