MountRoulette addon #1917
MountRoulette addon #1917
Conversation
| end | ||
| end) | ||
|
|
||
| windower.register_event('addon command', function(command) |
There was a problem hiding this comment.
While this pattern by itself is nice for more complicated command handlers I feel it's unnecessarily complex here. I'd just make //mr run the roulette and remove the help text and all the boilerplate surrounding it, as well as the help message for it. I don't see a benefit to having this setup,
There was a problem hiding this comment.
No worries - this is the boilerplate left over from removing the blacklist stuff. I'll cut this out as well.
| end | ||
| end | ||
|
|
||
| windower.register_event('load', function() |
There was a problem hiding this comment.
I would probably forego the load event here and just call the function directly. Its only purpose is if the code should execute after the full file has been loaded, but that does not seem to be necessary here.
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
I think this function needs to clear the allowed_mounts every time it's called. Otherwise if you log into a character that has all mounts, then log out and into a mule that has none, it will still have all mounts unlocked, no? Or am I missing something here?
There was a problem hiding this comment.
You're absolutely right, I'll clear it.
| local mount = possible_mounts[mount_index] | ||
|
|
||
| -- Add this to allowed mounts if it is not already there | ||
| if not allowed_mounts:contains(mount) then |
There was a problem hiding this comment.
If the allowed_mounts are cleared at the top of the function this check could be removed, right? That said, if that was made a set (S{}) you could remove it regardless, which is also what I would do, even if it's temporary and converted to a list later:
allowed_mounts = L(allowed_mounts_set)|
|
||
| windower.register_event('incoming chunk', function(id) | ||
| if id == 0x055 then --ki update | ||
| update_allowed_mounts() |
There was a problem hiding this comment.
Have you verified that this works once you get a new mount? I don't think it should, since the received KI packet will trigger the function, but at that point it hasn't reached the game yet, so windower.ffxi.get_key_items() would not know about it, I think. You may want to run it in a slightly delayed coroutine:
update_allowed_mounts:schedule(0.2)There was a problem hiding this comment.
Ahh I did not know that - I've probably not ran into this issue due to getting new mounts in Upper Jeuno, then needing to go outside to summon a mount. Given this, do still think it's worth the schedule call?
There was a problem hiding this comment.
Thinking about it, EmpyPopTracker has this same pattern and works ok. Is it more of a race condition?
There was a problem hiding this comment.
Actually, now that I think about it this should work after all. We store our own copy of the key items, and windower.ffxi.get_key_items returns that, not the game's key item store, so it shouldn't matter. At least I think so...
If you have some mounts left to get you could test it by the function print out the mounts it has found, then you'll see if it's really there.
There was a problem hiding this comment.
I think an alt of mine has some mounts to turn in so I'll check that soon.
There was a problem hiding this comment.
Thankfully it seems to work fine:
Removed unnecessary load event. Removed unnecessary command complexity. PR comments.
|
One thing I haven't considered is if someone uses this on a character with zero mounts. You'll get an error when it tries to concat the /mount with the mount name - should I cater for this? |
|
Ah why not, it's good form. |
|
Ok cool, I just threw a return statement in there rather than logging something. |
Summons a mount at random based on your mount key items.
A function to blacklist certain mounts exists on another branch and isn't included in this PR because of the impact of #1916 (blacklists mounts unexpectedly across multiple characters).