Carbonite breaks SetMapByID/SetMapToCurrentZone
Hi,
in the spirit of cooperation with other addons that rely on some of the map functionality to actually work, I would kindly ask you to remove the global replacements of the SetMapByID and SetMapToCurrentZone functions. Replacing them with your own implementations has severe consequences on other addons that try to interact with the map, or even simpler, just try to interrogate the map for details about the zones. At the very least, please ensure that the replacement SetMapByID actually matches the behaviour of the original - specifically, the original returns "true" when a map change was successfull, the Carbonite replacement never has any return value. I would still strongly urge you to stop replacing global WoW API, not for you, but for your users, as otherwise using it with many other addons that may not even work with the map directly, but only with coordinates obtained from the map, becomes impossible. - Nevcairiel |
I dug up some more details. The hooks of those functions were added in December '14, so not too long ago:
https://github.com/Rythal/Carbonite/...322039fc48b8ac Since this is relatively new code, maybe finding an alternate way to fix whatever this was supposed to fix is quite possible? |
Yes u are right... i saw this too, can u point me to source code of orginal functions (SetMapByID and SetMapToCurrentZone)?
|
Those are WoW functions, there is no source.
|
As an aside, isn't the following code:
Code:
if level then Code:
blizSetMapByID(zone,level) |
Yes. (too short)
|
Not necessarily, some APIs behave differently if a parameter is present and nil, or not present at all. I'm not sure if this is the case here, however.
|
Quote:
|
Quote:
Quote:
As to SetMapByID which is replaced by Rythal: could we use hooksecurefunc here? And attach to SetMapByID rather then replacing it? |
Quote:
|
Quote:
Its a good way to test if a given map id is actually valid. Most of these docs are derived from ingame tests, so it should just be augmented. Regarding hooking, as Nimhfree pointed out, that wouldn't be able to fullfill the same goal as the current solution (whatever that goal is). However, I would argue that if an addon calls SetMapByID, it has a reason for that, and it shouldn't be blocked from doing so. My guess is that its a performance optimization to stop other addons from redrawing the map while Carbonite is doing its thing. However, there are better solutions than entirely blocking this. For example, when my addon switches the map around just to get info from the map API, it'll disable the WORLD_MAP_UPDATE event before, so that other addons would never know that I even called it - avoiding any extra load. |
Hey, how do you disable the posting of that WORLD_MAP_UPDATE because I would love to do something similar when I am reading maps. Of course no one has complained as of yet, but you never know.
|
Quote:
|
Lua Code:
Call UnregisterWMU before doing map things, call RestoreWMU when you are done (and preferably after you restored the original map again, so addons dont get confused) Obviously this should be used with care and only in singular code blocks, so that the event doesn't stay unregistered for longer than your code needs to run, and addons dont break. I could totally imagine that one of the reasons for these performance optimizations referenced in the commit I linked earlier was an addon doing map changes without such a trick. For example, HandyNotes (which is actually the reason for this thread), had quite a few bugs which caused excessive map changes in some areas (mostly due to the broken Astrolabe library) - but a lot of map functionality in HandyNotes has been re-written and cleaned up, and Astrolabe replaced. |
It actually wasn't done for Carbonite's sake, it was done because I (and others) found the entire game would crash when using Carbonite and other addons which also utilized the map (*cough* Zygor *cough*) and I narrowed it down through stack trace print debugging to be those 2 function calls being called thousands of times in seconds by that particular addon triggering the lua anti-spam.
The point of the replacement was to abort any call to set the map if your trying to set it to the same zone already being displayed which immediately stopped that problem from happening. I am definitely open to suggestions of how to deal with other's bad code :) |
And actually reading more on it, i wouldn't be surprised if astrolab is the main culprit of the problems I was having... since Zygor also uses it.
|
Its certainly possible, Astrolabe misbehaved quite a bit in 6.x, it couldn't deal with some of the flukes of the map API.
|
I still don't have a computer to do large changes but I will try and do some editing from my tablet to try and do a couple things to alleviate this
1) I'll make the altered versions return a value 2) I'll by default not take over the global function, and add an option for people to enable if they wish to enable it / using addons with outdated astrolab and having problems. |
Sounds good to me! Returning a value should probably result in most functionality working already - and making the overrides opt-in is the icing on the cake. :)
|
Dev version just pushed for testing... has both of these changes (and warehouse gold count) will move it to live if no one reports anything breaking from it.
|
Quote:
EDITED: I forgot to mention that the error I was getting was when I used both Zygor's Guides and Carbonite's Quest Module at the same time. I am now using both of them simultaneously with no error message. |
One of the 2 longstanding issue's should now be resolved, and dailies should follow / obey the add new quests rules.
It amazes me sometimes how 4-5 lines of code can take days to finally get right and fix a problem :| Next i'm looking into the bonus quests coming up as completed. |
Bonus tasks saying complete is now fixed.. thankfully it was very tiny issue to resolve.
|
Thank you for your continued effort.
|
All times are GMT -6. The time now is 05:51 PM. |
vBulletin © 2024, Jelsoft Enterprises Ltd
© 2004 - 2022 MMOUI