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

Skip first baking in ModelLoader #3621

Merged
merged 4 commits into from Jan 12, 2017
Merged

Skip first baking in ModelLoader #3621

merged 4 commits into from Jan 12, 2017

Conversation

williewillus
Copy link
Contributor

@williewillus williewillus commented Jan 11, 2017

Overview

Simply put, skips baking models the first time around.
I structured this similar to Lex's "skip first stitch" optimization back in 1.8.0.

Similar to how the "skip first stitch" optimization still loads textures, but just prevents them from being added to the atlas, this optimization is also fairly conservative in that the models are still loaded and parsed, but are simply not baked. This will still bring a substantial speedup to large packs as the baking time dominates both model loading and texture stitching - there are many more actual baked models than json and texture files.

Things that I think will be FAQ:

Q: Why is firstLoad static?
A: A new ModelLoader instance is created every resource reload, so it can't be an instance variable

Q: What about ModelBakeEvent?
A: It is still fired after the first bake, just like how TextureStitchEvent.Pre/Post are still fired even if the first stitch is skipped. That's what lets this PR be so small, and not need vanilla patching.

Q: Why put the baked missing-model in everything before returning?
A: It serves as the default value of the bakedRegistry that is returned, and later parts of the loading process assume it's present. Also some mods' ModelBakeEvent handlers assume that their model is already in the registry and that it's not null, so just to be safe we put it the missing baked model into every MRL so far.

Open things I'd like feedback on:

  • Should I stick a comment in saying what's happening?
  • All rendering in the Forge dev environment appears to still work (vanilla + all test mods. I also tried RFTools), but this should be tested with other mods.
  • "Skip first stitch" has a JVM flag to disable it. Should this too?
  • IMO we should Cherry pick this to 1.10.2 since there are big packs on 1.10.2 that would greatly benefit from this (SF3, Infinity "Lite", etc)

@liach
Copy link
Contributor

liach commented Jan 11, 2017

Is this a performance pull request @mezz?

@mezz mezz added 1.11 Performance This request deals with performance, whether reporting issues or claiming to improve it. labels Jan 11, 2017
@mezz
Copy link
Contributor

mezz commented Jan 11, 2017

Looks good to me, and works with a few 1.11.2 mods I was able to test with in dev.

@mezz
Copy link
Contributor

mezz commented Jan 11, 2017

Should I stick a comment in saying what's happening?

No need, it's pretty straightforward. That said, adding a comment would not hurt.

All rendering in the Forge dev environment appears to still work (vanilla + all test mods. I also tried RFTools), but this should be tested with other mods.

Tested a few and it looks fine but there aren't many mods out for 1.11.2.

"Skip first stitch" has a JVM flag to disable it. Should this too?

I don't see the point, personally.

IMO we should Cherry pick this to 1.10.2 since there are big packs on 1.10.2 that would greatly benefit from this (SF3, Infinity "Lite", etc)

Sounds good to me, 1.10.2 packs do seem to take ages to load. We'll see what Lex says since it's debatable if this is considered a serious bug or not.

if (firstLoad)
{
firstLoad = false;
for (Entry<ModelResourceLocation, IModel> e : stateModels.entrySet())
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well use stateModels.keySet() if the values aren't needed.

@mezz
Copy link
Contributor

mezz commented Jan 12, 2017

Lex wants a JVM flag, otherwise should be good.

@williewillus
Copy link
Contributor Author

williewillus commented Jan 12, 2017

JVM flag added similar to the other one (fml.skipFirstTextureStitch -> fml.skipFirstModelBake). cherry pick to 1.10 pls :D

@LexManos LexManos merged commit a412886 into MinecraftForge:1.11.x Jan 12, 2017
@williewillus williewillus deleted the skipfirstbake branch January 12, 2017 02:39
AtomicBlom added a commit to AtomicBlom/HomecraftMineware that referenced this pull request Jan 13, 2017
mezz pushed a commit to mezz/MinecraftForge that referenced this pull request Jan 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.11 Performance This request deals with performance, whether reporting issues or claiming to improve it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants