Bug - /face heading fix

[AmericanNero]

Dear MQ2 Deities. I would like to address an issue, and provide the fix.

First the annoyance. Then it's all unicorns and rainbows.

The simplest case to reproduce:
1) /face heading 0 - this points you due North
2) /echo ${Me.Heading.Degrees} to see. It should return 360.00
3) /face heading 360.00 - works fine. This is aka 0.00.
3) /face heading 20. It should turn you 20 degrees to the right. But it doesn't.
4) /echo ${Me.Heading.Degrees} will now show you 340.00 (which is correct - degrees increase clockwise)
5) /face heading 340.00 - whoa whiskey-tango-foxtrot! it spun you around to the other 340.00.

The code converts the degrees float into a range from 0-512, which I presume is how eq stores it.

A value of 0 or 512 corresponds to 0 or 360 degrees.
128 corresponds to 90 deg
256 to 180 deg
384 to 270 deg.

The conversion would be:

1) Degrees mod 360
2) Degrees * 1.422222...and so on.

[CODE lang="cpp" title="Old and Busted - Line 2365 MQ2Commands.cpp"]gFaceAngle = Heading / 0.703125f;
if (gFaceAngle >= 512.0f) gFaceAngle -= 512.0f;
if (gFaceAngle<0.0f) gFaceAngle += 512.0f;[/CODE]

[CODE lang="cpp" title="New Hotness"]gFaceAngle = Heading % 360.0f;
gFaceAngle *= 1.422222f;[/CODE]

AN

 

[vsab]

Merge request on https://gitlab.com/macroquest/next/mqnext ?

 

[AmericanNero]

I have not made changes to the source. I trust someone more experienced with maintaining the code will do that.

 

[vsab]

I have not made changes to the source. I trust someone more experienced with maintaining the code will do that.

That's why the code gets reviewed by a maintainer before they allow it to be merged Go for it!

 

[AmericanNero]

I made the change to mq2command.cpp and committed it. What am I supposed to do next?

 

[vsab]

They don't have a contributing.md (which normally details the process and has guidelines too), but I would follow the normal Gitlab merge request process https://docs.gitlab.com/ee/user/project/merge_requests/creating_merge_requests.html

 

[brainiac]

Issues · macroquest / next / MQnext

The next major iteration of MacroQuest

gitlab.com

 

[brainiac]

Had a chance to look at this. Not going to make any changes, as the gFaceAngle is expecting 512 unit circles, not 360 degrees. Your change can potentially break existing code.

There is a reason your numbers aren't matching. You're inputing 512 "eq" units and converting it to degrees in the output.

Perhaps what we want is a /face command that accepts degrees.

 

[Sic]

Had a chance to look at this. Not going to make any changes, as the gFaceAngle is expecting 512 unit circles, not 360 degrees. Your change can potentially break existing code.

There is a reason your numbers aren't matching. You're inputing 512 "eq" units and converting it to degrees in the output.

Perhaps what we want is a /face command that accepts degrees.

you can /face heading # (which you know and was mentioned before)

would that be different ?

 

[brainiac]

you can /face heading # (which you know and was mentioned before)

would that be different ?

/face heading expects 512 eq units.

There isn't currently a command that accepts degrees for /face at the moment, but I Can add one.

 

[Sic]

/face heading expects 512 eq units.

There isn't currently a command that accepts degrees for /face at the moment, but I Can add one.

face heading 0 always goes north, 180 always south, 90 west, and 270 east

 

[brainiac]

Actually, you're right -- I was reading the code and missed the conversion to 512 units. it does take degrees.

 

[dannuic]

To be clear, this is entirely just a matter of convention. The current code is correct in that conventional heading convention dictates that positive heading values indicate "to the west" and negative heading values indicate "to the east" -- where negative is equivalent to greater than 180.

EDIT: I'm wrong about the convention for the definition of "heading", I was thinking of direction angle (which is measured counterclockwise from positive x). But the point here is that you're wanting to change the convention for something that has existed for a while, which is potentially breaking.

 

[brainiac]

Actually, you're right -- I was reading the code and missed the conversion to 512 units. it does take degrees.

I need to review this when I'm not distracted by work. I'll do a once-over all the facing code.