saif-ellafi / foundryvtt-mythic-gme

Mythic GM Emulator Macros and Roll Tables
Other
27 stars 15 forks source link

Rolling on oracle tables is slow #30

Closed pwim closed 1 year ago

pwim commented 1 year ago

When I click on oracles like "Actions", it immediately adds a card to chat. However, it takes about 7 seconds before it is updated with the first word, and about 2 more seconds before it is updated with the second word.

saif-ellafi commented 1 year ago

Hi @pwim - indeed this is a feature, not a bug :P, for when you use 3D dice (this is the case, no?). With Dice so Nice, it will wait until 3D dice roll, before it adds the words to the chat. This is not to spoil the 3D dice rolling animation.

pwim commented 1 year ago

I don’t use any 3D dice plugin though. I took a look at your code and saw a conditional checking for game.dice3d. Does this mean that if you aren’t using a plugin for 3d dice, there shouldn’t be a delay? If so I suppose another module I’m using could be messing with that value.

saif-ellafi commented 1 year ago

@pwim - interesting, yes, this delay is only triggered by 3D dice. Need to investigate then

saif-ellafi commented 1 year ago

For me, without 3D dice, it is also not immediate, but it takes about 1 second total to finish with both words

pwim commented 1 year ago

I confirmed it is an issue even if I disable all other module (except The Forge's required extension).

I'm running the latest version of chrome, but am on a M2 MacBook, so I suppose there could be something different about how it handles the JS (though I don't see why).

I tried stepping through code in the debugger, and through adding breakpoints, I think _mgmeGetAllPackTables is the culprit : https://github.com/saif-ellafi/foundryvtt-mythic-gme/blob/7a7552e8a6129c8c6d78cff7a73e9806f442e290/src/utils/mgme-common.js#L5-L13

For whatever reason, it appears that each line fetching the tables from a pack takes about 1 second. Maybe this is related to how The Forge is handling modules, in that I think they're changing things to the same assets can be used across multiple installs.

Regardless, perhaps there's some performance tweaks that could be made. As this method should return the same thing each time, I think you should be able to cache the result. Additionally, I think you could use something like Promise.all() to have all the promises execute in parallel instead of sequentially.

saif-ellafi commented 1 year ago

Hi @pwim - sorry I couldn't test and investigate this yet. But I think I know what it is, must be that The Forge handles the module packs outside of user space, so it goes over the network.

I would need to refactor the module so it minimizes the network calls over the compendium documents, which are the ones delaying for you

pwim commented 1 year ago

That's a bit frustrating that they handle modules differently resulting in issues like this. I just ran into another issue with the plugin, I'm assuming caused by a networking issue as well

Uncaught (in promise) TypeError: undefined. Cannot read properties of undefined (reading 'key')
[Detected 1 package: mythic-gme-tools]
    at generateOutput (mythic-gme-tools.js:1063:40)
    at Object.callback (mythic-gme-tools.js:1090:27)
    at Dialog.submit (foundry.js:56403:37)
    at Dialog._onClickButton (foundry.js:56332:10)
    at HTMLButtonElement.dispatch (jquery.min.js:2:43064)
    at y.handle (jquery.min.js:2:41048)

It was happening when I asked fate question of certain odds but not others.

I don't really think it is your responsibility to work around forge specific issues though, so this is more just for your reference.

saif-ellafi commented 1 year ago

Hey @pwim the second doesn't look like network issue, but will take a look. For the first one, i will try to find time to refactor the calls so if you import the compendium it avoids going outside through the network to retrieve the tables all the time

rikmarais commented 1 year ago

Hi there @saif-ellafi, Phi from The Forge here 👋

Thank you for looking into this. I believe that a single fetch at startup time and a caching of the value to be returned by _mgmeGetAllPackTables, with additional fetches to refresh the cached value whenever an important setting changes, or perhaps on a manual button click, should resolve the slowness that arises due to many network calls on a distributed storage environment.

Please let me know if there's anything you need from our side!

saif-ellafi commented 1 year ago

Thanks for coming across @rikmarais - will consider caching this, I thought of it too the issue for me was a bit memory consumption, keeping hundreds of tables in memory could have memory issues if not careful (i.e. leaks on a global variable).

Something a bit more practical, though more annoying for the users, would be to ask users to import the compendium tables into the local world, and then just change the logic to check local random tables presence first, before checking in the compendium, instead of fetching all together

pwim commented 1 year ago

I don't know if there's some difference between a system and a module with regards to compendium tables, but Ironsworn is quite similar to this module, having a lot of Oracle tables in a compendium, and yet doesn't suffer from the same delay.

saif-ellafi commented 1 year ago

@pwim - no worries, I am not saying it is your fault or the forge's :) it is definitely this module's problem. When I created this module, it only had very few tables, I never expected it to become so big, it is a design issue.

I am travelling, that is why I can't fix it so fast, but we'll have it fixed

pwim commented 1 year ago

Sorry, realized my comment forgot to mention why I was bringing up Ironsworn. I thought you might be able to take inspiration from however they're doing.

And thank you for this module! I've been using it in my D&D 5e game to great effect. I got the wonderful prompt of "Helplessly Broken" in my last session, which took it in a great direction.

saif-ellafi commented 1 year ago

Hi @pwim - I am back from my trip. I have published 2.8.4 which caches all mythic random tables now. Should definitely improve performance, I have measured RAM and it is not significant, so for now it should be fine even if not elegant. FYI @rikmarais

pwim commented 1 year ago

Performance is good now. However, I'm only getting a single result for oracles like "Actions", instead of a pair of words.

saif-ellafi commented 1 year ago

Let's track that issue in a separate issue! thanks