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:40 AM   #7
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   #8
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   #9
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   #10
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

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