[BeyondImporter] Slow performance

[Andargor]

The last run took about 1 minute to run, whereas before it would take a few seconds. Some changes seem to have slowed down the script considerably.

[derammo]

Developers: There are several things we can still do. Right now it is being paranoid about importing things slowly and deterministically (https://github.com/RobinKuiper/Roll20APIScripts/pull/18). This means that especially simple characters with few attacks import a lot slower (while those with many attacks may be faster?) To be honest, I never measured where the time is spent, because I was just trying to get something deterministic/testable and prevent the triggers from overwriting our stuff.

1. I think we should go ahead with the experiment of silent writing everything and then selectively writing the major trigger attributes (not silent) to see if that is safe. Also, who knows if sheet triggers actually get suppressed on the key attributes. Much testing required.

2. It is possible that deterministic writing isn't necessary at all, and I will test this. Perhaps 100% of the problems I was trying to fix were actually just the sheet upgrade overwriting things. So maybe it will be fine going back to a giant write, even though it seems crazy to let all these triggers run when things aren't populated yet. Maybe there is a sweet spot around writing class, level and multiclass attributes (the list of triggers from OGL update_class) serially and then globbing everything else together.

[sillvva]

I think it would be fine to:

1. Serially write the class, level, and multiclass attributes. Including sheet version.
2. Mass write everything but spells.
3. Continue writing the spells in blocks of five.

Let's give this a try and see if it retains import integrity while improving performance.

[derammo]

Yes. Not to brag :) but I planned ahead for this. Right now, everything that I THOUGHT is harmless in terms of triggers is in "single_attributes" and "save_proficiency_attributes". We can combine these back into a block write after making sure there aren't any major triggers in there. That's why I want to get the list of update_class trigger attributes from the html and actually put them in the code to make sure we are not including them in that set.

I am not going to be able to work on this stuff until 3 days from now. But if i is still open at that time, I will add time measurements to the code to see how long each phase takes and do the checks described above. Then we can make an educated decision of what to short cut.

The full silent write thing is a separate experiment, I think.

[sillvva]

I can work on it in later tonight. And yeah, I think the silent write should be a separate experiment for now.

[Andargor]

I wasn't complaining particularly about the speed, 1 minute is not too bad. However, I didn't get any feedback from it that it was working, so less technically savvy players might think it didn't work. So I suggest some kind of progress whispered to the player would be good.

EDIT: Also, the longer it takes the more chance that two players do it at once, is that a problem?

[sillvva]

We can definitely add a post to chat that the import has started.

We might have to double check to make sure two characters being imported at the same time causes no issues. I think there could be some conflicts, actually.

[derammo]

Yes, there is a commented out chat message when it starts that I put there asking if we should have that :) but it didn't make it into the release. I think we should just say that an import is starting. I thought about locking out additional imports, but I was worried that the lock might not be cleared if it hits a bug and just stops.

Incidentally, I started rewriting stuff to have a separate context for multiple imports (making things into classes etc.) https://github.com/derammo/Roll20APIScripts/blob/classes/BeyondImporter_5eOGL/BeyondImporter.js

then I decided that

1. that's rude to do to someone else's code :)
2. it isn't worth the effort at this point

[sillvva]

Did a test run on a few characters with the method mentioned above. And everything looks good so far. Spellcasters could still take a bit of time depending on how many spells they have. However, non casters have a very quick import time with no issues from what I can see.

[derammo]

please see https://github.com/RobinKuiper/Roll20APIScripts/pull/37