remance / Masendor

Open source educational and historical battle action game, All helps accepted
MIT License
142 stars 31 forks source link

Improve loading speed and output #36

Closed coppermouse closed 1 year ago

coppermouse commented 1 year ago

No need to merge this yet, just wanted to give you an idea of what to improve.

(also give any feedback you want in regard of code, commit-messages, pull request/branch name and so on)

(I also branch from an older version than main because main does not work)

I want to look into how to make load time faster, they are more than one minute on my computer.

During load time the game appears to be frozen, the most optimal solution would be to have a load information output on the screen if the load times are more than 2-3 seconds IMO.

What I done here is that I made some output but it is at the moment stdout only.

Loading weather...  DONE (0.0002579689025878906s)
Loading music...  DONE (0.00016570091247558594s)
Loading map events...  DONE (0.0004496574401855469s)
Loading images...  DONE (2.659607410430908s)
Loading draw map...  DONE (2.935182571411133s)
Loading common setup...  DONE (0.27946901321411133s)
Loading sprites...  DONE (63.075032472610474s)

We can see that load sprites is the big one. A easy solution (hopefully) is to cache all the sprites on disk. That would make it only take one minute the first time.

remance commented 1 year ago

Yes, your suggestion of caching should help a lot with loading time and it is something I did consider a bit but got no time to implement yet (same with loading screen).

The commit name look good to me and is informative enough.

For the coding style itself, I think for this commit it look fine but I did find the way you did in viewer.py a bit weird. Is that a common coding method (maybe for different language/python version? at least it is different from PEP)?

Why is the main branch not working though?

From your comment in the previous discussion:

34 """

About being a collaborator, what are the rules? I can push anything as long as I do it in my own branch? Do you have a document for this? """

There is really no exact rules and document for collaborator yet since most of the external help were kind of minor so far. So it depends on how much you want to help, what aspect and how often I guess. We can set up a way to discuss the collaboration on something like discord, github discussion, or even pidgin to keep thing simple if you prefer.

coppermouse commented 1 year ago

@remance main does not work because it is missing troop_status.csv (far as I see, it might be more files missing)

I could run my code through a PEP-validator. I will look into it.

remance commented 1 year ago

I think I did get that error about troop_status.csv error as well when I tried your viewer.py at first. I think it is due to ruleset being set at 1. Check if you have this self.ruleset = 1 in the code (not sure why would it be like that on yours), and change it to 0. I changed the ruleset number system a bit in previous update.

coppermouse commented 1 year ago
Loading weather...  DONE (0.00017690658569335938s)
Loading music...  DONE (0.00016617774963378906s)
Loading map events...  DONE (0.0004477500915527344s)
Loading images...  DONE (2.6645872592926025s)
Loading draw map...  DONE (2.927405834197998s)
Loading common setup...  DONE (0.3470580577850342s)
Loading sprites...  DONE (0.369992733001709s)

Today was a success, loading sprites down to 1/3 of a second instead of 1 minute. Of course this is only after you loaded the level once before.

There is a lot of comment and discussion I want to do regarding this solution. It is not done, I think there are some things that can be done better, refactor and clean the code and so on but I will have to come back to this tomorrow.

remance commented 1 year ago

Thanks a lots! I have some questions as well that probably better to do it somewhere else than here. Should we setup direct message so it is easier to chat?

By the way is the game code as it is understandable enough, or should there be more comment/documentation?

coppermouse commented 1 year ago

Yes, you can decide on what type of communication you want.

Until you do that I have some few questions / comments:

  1. I think the code solution would be easier create a class like this: https://github.com/pygame/pygame/issues/3054#issuecomment-1156481897 . If we do that we do not have to switch surfaces in the dictionary before and after save. It will be a lot easier.
  2. When I make a new class or methods, should I create a new file for them?
  3. I actual wanted to add the "private" methods to the create_troop_sprite_pool since they are only being used there, so why bother making a new file, but I could not import them. That is why I had to do a create_troop_sprite_pool_methods file. Do you know why? Try import inner_create_troop_sprite_pool from another location and see if that works, if that doesn't, do you know why?
  4. The most optimal cache method would be to cache every sub unit by themself but that was a bit harder to do. Now we cache by set of sub units (pool). Also cache file gets very big (384mb).
  5. Name and location of the cache files? suggestion?
  6. If I cache sprite for a sub unit (like "L36") will they always look the same or are there external things that can make them look different? If they can look different cache must take those external things into account.

You do not have to answer everything at once. Why I wait for you to set up a communication channel I will look into PEP-validators.

remance commented 1 year ago

Do you have a discord? just join the one in the readme.md then we can just chat there in direct chat. The another chatting system I know of is Pidgin but dunno if you used it before. If not, we can actually keep doing it like this as well.

Answer to each point:

  1. I am not that familiar with pickle caching, but as long as this does not affect the code structure and performance then I see no reason to object with what you recommend. From what I understand the class that you want to create will be only for when the game load cache data during sprite cool creation and not loading to play it during gameplay right?

  2. It really depend on how large and relatable of the class/method. The current way I do it is that small object class (like UI button and stuff) are keep together while the one with a lot of methods like subunit is kept in its own file with large method as its own separate file as well (in common/subunit folder as you can see). If the method is a bit more general then I keep them in common.utility (safe to assume the caching method you are making can be used for other stuffs as well? like map sprite)

  3. Not quite sure what you mean here. Do you mean why I make create_troop_sprite_pool as its own file?

  4. Actually I was wondering about whether it would be better to just cache everything once the game start. Is this 384mb include every single subunit in the game? or just 1 battle. Also how did you compress the cache?

  5. I think cache for the folder name should be fine. For the cache location, I remember seeing it written by someone that writing data in game folder is not always the best option since some system permission can forbid it that why a lot of games save cache and other stuff in document or roaming, is that true? My initial answer would be just keep it in the game folder for now.

  6. It is something I wanted to discuss as well. The thing is that the game data and mechanic is not really finallise yet. So the caching will need to keep track of possible change for sure. For example, the part sprite image may get improved later or troop stat got changed. Another thing is that the game intend to be easily moddable, that why all of the data is kept in csv file so anyone can edit it directly.

coppermouse commented 1 year ago
  1. No, The new class PickableSurface( pygame.Surface ) will end up being used almost everywhere. But since it is a subclass of pygame.Surface it will still be a normal surface (but also able to be stored in pickles).

But since we also can run pygame.Surface = PickableSurface all reference to Surface will work as before.

I will try this myself just to make sure

coppermouse commented 1 year ago
  1. common.utility? Perhaps that is where I should have put my save_load_dict_with_surfaces.py. Not sure how reuseable my code is.
  2. No, but that does not matter at the moment, we can take it later if question comes up again
  3. 384mb is for the battle only, sadly. Good point, I do not think I compressed the data. Since I use "tobytes" I do not think it make use of compression. It can be an easy fix.
  4. Now all the cache ends up in its own folder. I create the folder if not exist when I need the cache. Location: cache/cache_MD5.pickle. Not sure about the permission you asked.
  5. The correct way to cache something is do it based on the data it is using, not the "name" of the data. Yes, this can be an issue if people modify stuff and doesn't know cache is a thing. Perhaps cache is for developers only for now, you know, those who has restart the game a lot of and test stuff (and know how to clear the cache if needed) :)