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

Model cache #185

Closed
wants to merge 19 commits into from
Closed

Conversation

lheckemann
Copy link
Contributor

@lheckemann lheckemann commented Sep 27, 2017

PRing for attention/comments/utility to people even though it's not release-ready. This is based on (and has a small dependency on) #86, but the key addition is 41250d4. This change comes with a number of caveats, documented extensively in the commit message, but the benefit is that it makes Barony load 7 times faster once the cache has been generated on the first launch.

Since they are inlined, their implementation needs to be included in
every source file that uses them, otherwise it will go missing entirely
and fail to link.
cppfuncs.hpp:getLinesFromFile → files.cpp:getLinesFromDataFile

Also rewrite most of its implementation to provide useful errors
when the file cannot be opened.
- Add the openUserFile function, analogous to openDataFile, which opens
  files containing user data, i.e. config, saves and scores.
- Make the default location for user files $XDG_CONFIG_HOME/barony or
  $HOME/.config/barony if those variables are found. This should
  probably be replaced with behaviour more suitable for OSX
  ($HOME/Library/Application Support/barony?) and windows
  (%APPDATA%/barony?)
- Add -userdir= option which allows overriding the aforementioned
  behaviour
- Add setUserDir and setDataDir to the files module, so the knowledge of
  the static storage for these paths can be limited to files.cpp.
  Correspondingly,
- Remove public datadir variable
Move all openal/FMOD-specific functionality into
src/sound_{fmod,openal}.cpp. Now everything else should depend on the
common backend-agnostic interface provided in sound.hpp and the backend
is determined purely by linkage.
and add -Werror=write-strings so they won't be reintroduced!
typo wasintroduced in f7d96cd
This improves startup performance significantly, in my case reducing it
from 15 seconds to 2. However…

This is naïve in several ways, and has various serious failure modes and
as such should not be used as-is in a release version. It does however
lend itself to a much faster development cycle.

This does not take any changes to model-related files into account:

 - If models.txt is extended, it will likely crash
 - If models are reordered, the models displayed in-game will be wrong
   in potentially hilarious ways.
 - If model file references are changed, the changes will not be taken into
   account and the files used at cache generation time will be used.
 - If model files are modified, the changes will not be taken into
   account.

All of these issues can be remedied by removing the model.cache file
generated in the user data directory when any model files are altered.
As a hack for development purposes, the models.cache file could be
created and set to be neither readable nor writable by the user running
the game in order to prevent it from being used.

In addition, the file format is dumb and an incorrect models.cache file
will likely crash the game. It may also introduce security
vulnerabilities, so models.cache should never be loaded from an
untrusted source.

To improve on this, it would probably make sense to use a hash-based
data structure to map the contents of an input model file to its polygon
representation consistently. This would allow handling changes to the
models transparently while still providing a significant performance
boost. Adding compression may also be worthwhile, as the cache file ends
up being around 44MB with the data from the current release.
@addictgamer addictgamer added this to the Barony Next milestone Oct 20, 2017
@WALLOFJUSTICE
Copy link
Collaborator

This is great! I just cherry-picked the Cache models commit 41250d4 and edited the openUserFile with the old openDataFile to get it running.

I've added locally /usemodelcache as a flag in default.txt to decide whether to use the cache, and /disablecache in game to clear the flag for the next run.

All that's missing is a rebuild section in game (something like /regenmodelcache then I could be content with it. Having an optional flag in default.txt (false by default - user must enter command or edit the file manually) makes it OK to pull IMO.

@WALLOFJUSTICE
Copy link
Collaborator

WALLOFJUSTICE commented Oct 28, 2017

My work is on the models-cache branch, commits 335718e and 74227f0

My command /loadmodels 400 450 reloads the model files for indices 400-450, leaving the other stuff intact + without restarting Barony. I added a line when this command was used to rebuild the cache, so now you can edit your models, /loadmodels in game to see the changes, and on next startup the updated models.cache file will be loaded and reflect the current state of models.txt

Only thing left would be handling of extending models.txt, unsure of the consequence off the top of my head

@lheckemann
Copy link
Contributor Author

lheckemann commented Oct 28, 2017

What may make more sense is to ship individual model files in the cached format consisting of triangles, or append the triangles to the voxel model files, rather than storing them all in one big cache blob file. I'll draft something these next few days.

@addictgamer
Copy link
Collaborator

addictgamer commented Nov 23, 2017

rather than storing them all in one big cache blob file.

Any particular reason why you'd not prefer that route? I'm fine with one blob file if it means one burst disk read rather than several hundred separate file accesses.

@lheckemann
Copy link
Contributor Author

The main issue with the cache blob file is that it doesn't keep the polygon data in a relationship with its voxel "source" data, which means we'd have to mess around with hashes and stuff, or suffer the longer original load times when modifying the models (and there's no real technical reason for that).

As for disk access, it's not a noticeable factor in load times for Barony. Keep in mind that all the file loading is done before the progress indicator for models is displayed, so what we're addressing here is very much a CPU/memory-bottlenecked process.

@addictgamer
Copy link
Collaborator

Keep in mind that all the file loading is done before the progress indicator for models is displayed

Ah, nevermind that then.

@lheckemann
Copy link
Contributor Author

Integrated in ec76d92

@lheckemann lheckemann closed this Dec 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants