XCompWiz / Mystcraft-Issues

Issue tracked and localization repo for the MInecraft mod Mystcraft
xcompwiz.com
31 stars 22 forks source link

Oddball crash encountered while playtesting #283

Closed Sunconure11 closed 6 years ago

Sunconure11 commented 6 years ago

https://paste.dimdev.org/fuwuwihezu.mccrash

XCompWiz commented 6 years ago

@HellFirePvP, looks like I broke something in the page texture stitching? Is it that something isn't being stitched properly and then is being requested later? How is the lectern provoking a texture stitch request?

Sunconure11 commented 6 years ago

It's related to texturefix. I got another error.

https://pastebin.com/iSSKYn4t

https://minecraft.curseforge.com/projects/texfix

XCompWiz commented 6 years ago

Ah, it's restitching later... that's... weird and wrong.

Sunconure11 commented 6 years ago

@Speiger

Speiger commented 6 years ago

@XCompWiz basically texfix unloads all texturedata from the CPU ram that aren't needed (basically unanimated ones) because they are stored in the GPU memory where they are in 90% of the cases only used, and if a mod tries to access the texture data (on the cpu side) it reloads the nessesary data in that moment and marks them to unload in the next game tick, This reload mecanism is what your texture causes to crash, its not a illegal state since that function call moves the texturedata from a file into the ram and can be called any time, it has nothing todo with the stitcher itself. Besides the reload mekanism this feature is implemented in vanilla and disabled by forge which basically bloats the textures memory footprint, especially in modpacks where you have tenthousands of textures.

Edit 1: Just some detail addition.

XCompWiz commented 6 years ago

Fair enough. The page TextureAtlasSprites are built such that they use data built during TextureStitchEvent.Pre and freed during TextureStitchEvent.Post. Forcing Mystcraft to rebuild that data on the fly could potentially result in counteracting your goal of freeing the memory, since it doesn't have a trigger to clean it up. Do you have suggestions on how that can best be handled? Secondly, if a mod is requesting the texture, shouldn't it be unloaded after some period of not being needed? So it doesn't get rebuilt every (few) frames?

Looks like the context for the texture getting reloaded is through renderItem, so it seems Minecraft effectively doing the reloading in this context. It would likely need to do that for item frames, yes? How does the repeated unloading play out there?

Edit 1: Question on item rendering.

Speiger commented 6 years ago

Oh i get it, you are pregenerating the Textures on the prevent and then deleting the cache on the "Post" event. That isnt really an issue. Your own system provides you a way to detect if its loaded on the fly or not. So if it is on the fly then you just load the nessesary data for that texture, and then clean your cache emediatly leaving just the texture data in the texture itself.

About the Unloading part, as soon someone requests a texture to be reloaded it automatically queues it up for unloading on the next tick. In other words it gets cleaned up emediatly after the use is over. TL:DR: I have a manager that handles the unloading.

ItemFrames do not access the texture data itself, they access the model. To give you a reference point what causes a reload: TextureAtlasSprite.getFrameTextureData(int index);

This methode is only called by the Models to gather the data for evaluation. My TexFix manager kicks in after all models were already generated (works also with resource reload). That means minecraft won't access the texture data ever again on these instances, when a resource reload happens the TextureAtlasSprite will be recreated anyway so my "Recreation Trigger" is deleted anyway, and will get added as soon my manager gets loaded after the ModelBuilder.

I don't use asm to add these features, that makes it a lot more bulletproof in my case.

Edit 1: LangFixes and asm info.

XCompWiz commented 6 years ago

I guess that's the simplest way, yes. I was hoping there was something slightly more elegant, but it'll do. ;)

And sounds good for the reloading, though I'd be of the mind to give it several frames before unloading data, simply because we might need it again soon-ish (recently accessed visuals tend to be frequently accessed within a short span of time, but not necessarily continuously). Either way, shouldn't cause any issues.

Thanks for being constructive!

Speiger commented 6 years ago

well its not exactly 1 tick, its 2 ticks to be precise, but i saw tinkers construct loading textures once if you open them in JEI and they usually load 5000 textures and having all these queued up for so long create lag to constantly check "do i need to unload" so its 2 ticks of a bit lag and then it unloads it again.

Also no problem.

XCompWiz commented 6 years ago

Makes good sense :D