Thread Tools Display Modes
08-23-15, 08:52 AM   #1
Nevcairiel
Premium Member
Premium Member
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 63
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
  Reply With Quote
08-23-15, 08:59 AM   #2
Nevcairiel
Premium Member
Premium Member
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 63
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?
  Reply With Quote
08-23-15, 12:33 PM   #3
ircdirk
A Molten Giant
 
ircdirk's Avatar
AddOn Author - Click to view addons
Join Date: Nov 2014
Posts: 823
Yes u are right... i saw this too, can u point me to source code of orginal functions (SetMapByID and SetMapToCurrentZone)?
__________________
Carbonite and Carbonite Classic Developer
  Reply With Quote
08-23-15, 12:55 PM   #4
Nevcairiel
Premium Member
Premium Member
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 63
Those are WoW functions, there is no source.
  Reply With Quote
08-23-15, 06:33 PM   #5
Nimhfree
A Frostmaul Preserver
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 267
As an aside, isn't the following code:
Code:
if level then
	blizSetMapByID(zone,level)
else
	blizSetMapByID(zone)
end
the same as:
Code:
blizSetMapByID(zone,level)
  Reply With Quote
08-23-15, 07:39 PM   #6
Seerah
Fishing Trainer
 
Seerah's Avatar
WoWInterface Super Mod
Featured
Join Date: Oct 2006
Posts: 10,860
Yes. (too short)
__________________
"You'd be surprised how many people violate this simple principle every day of their lives and try to fit square pegs into round holes, ignoring the clear reality that Things Are As They Are." -Benjamin Hoff, The Tao of Pooh

  Reply With Quote
08-24-15, 01:34 AM   #7
Nevcairiel
Premium Member
Premium Member
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 63
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.
  Reply With Quote
08-24-15, 01:40 AM   #8
semlar
A Pyroguard Emberseer
 
semlar's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2007
Posts: 1,060
Originally Posted by Nimhfree View Post
As an aside, isn't the following code:
Code:
if level then
	blizSetMapByID(zone,level)
else
	blizSetMapByID(zone)
end
the same as:
Code:
blizSetMapByID(zone,level)
It's the same as SetMapByID(zone), because it doesn't take a second argument.
  Reply With Quote
08-24-15, 04:43 AM   #9
ircdirk
A Molten Giant
 
ircdirk's Avatar
AddOn Author - Click to view addons
Join Date: Nov 2014
Posts: 823
Originally Posted by Nevcairiel View Post
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 was searching for it but nowhere is written that this funcition returns something, see here http://wowprogramming.com/docs/api/SetMapByID or here http://wowwiki.wikia.com/wiki/API_SetMapByID but ofcourse this can be outdated info...

Originally Posted by Nevcairiel View Post
Those are WoW functions, there is no source.
Many functions are available in FrameXML but i dont see SetMapByID there

As to SetMapByID which is replaced by Rythal: could we use hooksecurefunc here? And attach to SetMapByID rather then replacing it?
__________________
Carbonite and Carbonite Classic Developer

Last edited by ircdirk : 08-24-15 at 04:54 AM.
  Reply With Quote
08-24-15, 08:35 AM   #10
Nimhfree
A Frostmaul Preserver
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 267
Originally Posted by ircdirk View Post
As to SetMapByID which is replaced by Rythal: could we use hooksecurefunc here? And attach to SetMapByID rather then replacing it?
I do not believe so because there are code paths in the Rythal code that will cause the Blizzard code not to be executed. Hooking will always have the Blizzard code run.
  Reply With Quote
08-24-15, 08:53 AM   #11
Nevcairiel
Premium Member
Premium Member
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 63
Originally Posted by ircdirk View Post
I was searching for it but nowhere is written that this funcition returns something, see here http://wowprogramming.com/docs/api/SetMapByID or here http://wowwiki.wikia.com/wiki/API_SetMapByID but ofcourse this can be outdated info...
Just test it ingame, any valid map ID will return true, any invalid id will give you nil or false.
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.

Last edited by Nevcairiel : 08-24-15 at 02:04 PM.
  Reply With Quote
08-24-15, 07:19 PM   #12
Nimhfree
A Frostmaul Preserver
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 267
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.
  Reply With Quote
08-27-15, 02:40 AM   #13
ircdirk
A Molten Giant
 
ircdirk's Avatar
AddOn Author - Click to view addons
Join Date: Nov 2014
Posts: 823
Originally Posted by Nimhfree View Post
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.
Yes, im curious about that too...
__________________
Carbonite and Carbonite Classic Developer
  Reply With Quote
08-27-15, 02:41 AM   #14
Nevcairiel
Premium Member
Premium Member
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 63
Lua Code:
  1. -- unregister and store all WORLD_MAP_UPDATE registrants, to avoid excess processing when
  2. -- retrieving info from stateful map APIs
  3. local wmuRegistry
  4. local function UnregisterWMU()
  5.     wmuRegistry = {GetFramesRegisteredForEvent("WORLD_MAP_UPDATE")}
  6.     for _, frame in ipairs(wmuRegistry) do
  7.         frame:UnregisterEvent("WORLD_MAP_UPDATE")
  8.     end
  9. end
  10.  
  11. -- restore WORLD_MAP_UPDATE to all frames in the registry
  12. local function RestoreWMU()
  13.     assert(wmuRegistry)
  14.     for _, frame in ipairs(wmuRegistry) do
  15.         frame:RegisterEvent("WORLD_MAP_UPDATE")
  16.     end
  17.     wmuRegistry = nil
  18. end

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.

Last edited by Nevcairiel : 08-27-15 at 03:00 AM.
  Reply With Quote
09-02-15, 10:34 PM   #15
Rythal
Featured Artist
Featured
Join Date: Aug 2012
Posts: 1,458
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
  Reply With Quote
09-02-15, 10:39 PM   #16
Rythal
Featured Artist
Featured
Join Date: Aug 2012
Posts: 1,458
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.
  Reply With Quote
09-03-15, 03:24 AM   #17
Nevcairiel
Premium Member
Premium Member
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 63
Its certainly possible, Astrolabe misbehaved quite a bit in 6.x, it couldn't deal with some of the flukes of the map API.
  Reply With Quote
09-03-15, 08:24 AM   #18
Rythal
Featured Artist
Featured
Join Date: Aug 2012
Posts: 1,458
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.
  Reply With Quote
09-04-15, 09:03 AM   #19
Nevcairiel
Premium Member
Premium Member
AddOn Author - Click to view addons
Join Date: Aug 2006
Posts: 63
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.
  Reply With Quote
09-06-15, 02:11 PM   #20
Rythal
Featured Artist
Featured
Join Date: Aug 2012
Posts: 1,458
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.
  Reply With Quote

WoWInterface » Featured Projects » Carbonite » Carbonite: Dev Talk » Carbonite breaks SetMapByID/SetMapToCurrentZone

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off