Conversation
modloader/modconfig.py
Outdated
| mods[-1][3] += "\n\nVerified by {}".format(verified.username.replace("<postmaster@example.com>", "")) | ||
| else: | ||
| print "NOT VALID SIG", mod | ||
| print "NOT VALID SIG", mod[1] # Note: printing only the mod name, instead of the whi=ole thing SIGNIFICANTLY speeds up this call |
There was a problem hiding this comment.
This is slightly worrying - how many mods don't have a valid signature? I thought I'd signed most of them
There was a problem hiding this comment.
These 15 fail, though I haven't checked why
(3336106514) ALL WORKING MODS! 32 (Flight of Love doesn't show)
(3651582794) Angels With Scaly Wings: Chinese Simplified
(3666234554) Card Game Expanded 卡牌游戏增强(Two Languages Support)
(2801825863) Casual Arson
(2697982171) Casual Vandalism
(2096774388) Lorem_RPG
(2742645268) MagmaClient
(2990207385) Meet Naomi
(1305731599) Modtools
(2766323849) Name Re-entry
(3465212790) Naomi Maze Skip
(2736325690) Skip Credits
(2987066957) The Morning After
(3645564443) The Traitor
(3657449256) Vykoupení
There was a problem hiding this comment.
Most of those I can't remember anyone asking to sign, though I have signed 2 recently. The modtools itself is actually never signed so that's fine
There was a problem hiding this comment.
Ah, that's fair. I'm still hesitant to return the printing to how it was, As it seems to double the runtime of the validity check (from ~2 seconds to ~5 seconds)...
As for the modtools, I'll add a special case for it (to be ignored)
modloader/steamhandler_ex.py
Outdated
| # else: already done | ||
|
|
||
| # Write cache file | ||
| with open(cache_file_name, "wb") as cache_file: |
There was a problem hiding this comment.
Can we use literally anything except pickle? It's very insecure.
There was a problem hiding this comment.
Yeah. I used pickle because it was the simplest way to store and restore values so that they behave the same, but I have no attachment to it. I'm afraid I don't quite get what the security issue here is, so what would you suggest to use here? would a tuple of dicts be better (with adapted __getattr__ so that they behave like the ctype results)?
There was a problem hiding this comment.
Loading pickles can result in arbitrary code execution, but I guess the mod ecosystem is full of that. I'd still avoid it if we can. Why not say json?
There was a problem hiding this comment.
Oh, I forgot pickle can do that... json is seems to be the most standard way of dealing with this kind of data, so I guess we'll use that
modloader/steamhandler_ex.py
Outdated
| if not isinstance(steam_manager, SteamMgr): | ||
| raise TypeError("steam_manager must be a steam_workshop.steamhandler.SteamMgr instance!") | ||
| self._steam_manager = steam_manager | ||
| self.threads = [] # This can create threads in QueryApi, which will persist after the call returns (as is required). |
There was a problem hiding this comment.
This list only grows, never shrinks. Is this AI written?
There was a problem hiding this comment.
This was mostly not thought out well... Because of how QueryApi (and all of steam DLL functions) work, if the cache file is used then we need to create a thread to execute the query callbacks in order to imitate the behavior of steamhandler.QueryApi. These threads also need to live beyond the end of the function. I was concerned that these threads could be difficult to track (especially if some of the callbacks start misbehaving), so I thought "why not just chuck them into a list, so that we at least know which ones have been created?". but then I didn't really have much to do with them, so the list became kinda pointless.
Additionally, I avoid using AI for my work, so any stupid decisions are my own.
There was a problem hiding this comment.
I think the thread at least should self-clean itself up when it's done?
There was a problem hiding this comment.
Yeah, I should at least remove them from the list... as the threads themselves shouldn't have anything internal to clean... the multiprocessing ThreadPool would have been ideal, but multiprocessing is apparently not available, so I guess I'll just have them remove themselves when done.
| print "Called cached queryAPI with page={}".format(page) | ||
|
|
||
| # Get most recent cache file if exists | ||
| cache_file_name = self.get_cache_filename(page) |
There was a problem hiding this comment.
Why are we storing a cache in a file rather than say in memory?
Given that it expires in 15 minutes, that's not particularly useful for multiple play sessions
There was a problem hiding this comment.
The issue I was running into (which was the main one preventing the game from starting) Was that repeated calls to QueryApi failed silently, and in doing so, crashed the game. Once these start to fail, they keep failing even between play sessions until about 15 minutes pass, which prevents anything from accessing the modlist until that time passes. Therefore, I use a file to store those results, as they need to be accessible between play sessions.
I've added a description of this in the PR's description, as this is the most significant change.
There was a problem hiding this comment.
That's really weird and I'm not sure what to make of it to be honest
There was a problem hiding this comment.
Yeah... I only found it by trying to isolate the issues with CachePersonas (mainly why it crashes silently and abruptly), and found it really weird that QueryApi itself can cause it...
I do wonder if it happens just on my machine of is it a more widespread thing. I'd appreciate it if you can check if it also happens on yours (I did so by replacing the content of CachePersonas with a single call to QueryApi(1). Then I started the game, quit out of the main menu, then tried to start it again, which failed by this issue.).
It'd be quite interesting if it's just a localized thing on my machine...
modloader/steamhandler_ex.py
Outdated
| cPickle.dump(tuple(islice(array, arr_len)), cache_file) | ||
|
|
||
| except Exception as e: | ||
| print "Cache callback raised Exception:", str(e) |
There was a problem hiding this comment.
I honestly don't remember, I think it's a holdover from testing? We can reraise, or just remove the except case altogether.
There was a problem hiding this comment.
Just try/finally is probably fine here if there's nothing we actually want to handle
There was a problem hiding this comment.
If this callback fails I think it should be logged though? As it's a call that shouldn't fail under normal circumstances, and it's failure can be very problematic... But I guess I should still say as such in the code, as it currently seems quite random
There was a problem hiding this comment.
If the callback fails, it should probably cause an error screen to show up right?
There was a problem hiding this comment.
I guess so. is modloader.report_mod_errors appropriate for this case?
mods/core/download_mods.rpy
Outdated
|
|
||
| from modloader import modconfig | ||
|
|
||
| # Cache mod validity to expedite mod browser startup |
There was a problem hiding this comment.
I don't understand this.
There was a problem hiding this comment.
The call to modconfig.steam_downloadable_mods still takes a few seconds on the first call (as after that it's cached in memory), so I chucked it on a thread during startup so that it would already be done before the user ever wants to access the mod browser.
A more robust way of doing this would probably be to make a load_steam_downloadable_mods function which loads this data on a thread, then setup steam_downloadable_mods so that it waits until that function finishes. I honestly should have thought of that sooner...
… using the new SteamModlist class to manage this.
Added pagination to the in-game mod browser, along with fixing certain issues regarding to getting modlists from steam.
Main Changes
Fixed
steamhandler'sQueryApimethodThis method, which is used to get sections of the modlist from steam's API, has a tendency to crash silently on repeated calls, which also crashes the game. The source of this crash seems to be related to steam caching these results, as these crashes persist between play sessions, and occur consistently from the second call onwards for about 15 minutes after a successful call.
This was solved by created an on-disk cache of this method's results, and using that cache until that 15-minute interval elapses, which allows the
QueryApicalls to never use the problematic steam cache.Added pagination to the In-game mod browser
With the increasing number of mods available in the game since the development on the browser, both load time and navigation go slower as a consequence of the longer modlist. pagination was then added to both allow partial loading of the modlist, and to allow quicker navigation of the modlist.
Load speed improvements
The critical path of getting the modlist (which defines the content of the browser) has been examined, and certain optimizations have been made. of those there are: