[p5.js 2.0+ Bug Report]: Quaternion Multiplication typo/error

[roymacdonald]

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• WebGPU
• p5.strands
• Build process
• Unit testing
• Internationalization
• Friendly errors
• Other (specify if possible)

p5.js version

2.*

Web browser and version

all

Operating system

all

Steps to reproduce this

There is a typo in the p5.Quat multiplication;
In line https://github.com/processing/p5.js/blob/main/src/webgl/p5.Quat.js#L44 where it reads this.vec.z - quat.vec.z it should be this.vec.z * quat.vec.z. It is just replacing the - for *.
It quite easy to verify agains the textbook quaternion multiplication.
I am not sure about the implications this has in p5, as of how extensively this has afected p5 in general.
Not sure either if there have been other fixes to compesate this error in other places of the math.

[ksen0]

Thanks for flagging this! @perminder-17 would you mind investigating if you have time?

[perminder-17]

Thanks for catching this, @roymacdonald. I think the formula has a typo.

We used this reference while adding the code:
https://danceswithcode.net/engineeringnotes/quaternions/quaternions.html

The fix you suggested for the formula looks good to me.

I also noticed one small inconsistency. In the p5.js website documentation, the method is shown as mult(), but in the actual code you have to use multiply(). I think fixing this would also be a good part of this issue.

@method mult -> @method multiply (as per the actual api name).

[ksen0]

@roymacdonald do you want to make a PR or would you prefer this be opened to other volunteers?

[roymacdonald]

Hi @ksen0 I made. a PR, I thought I already did.

I don't think it is a good idea just to rename the function, as it might break other things. I think it should be another function and deprecate the old one.

[perminder-17]

Thanks! Just to clarify, we'll not rename any API. The actual method has always been multiply(), while mult only existed in the JSDoc @method tag. There is no real mult() method. This change just corrects the doc tag to match the real method name, and no code or tests use .mult(), so nothing breaks I guess? Do you have any suspect which could break after the tag name?

[roymacdonald]

I pushed into the PR a fix for the inline documentation.

I made a rough search over the codebase and all the other classes use mult for their multiplication function. So, in order to keep consistency, I think that this class should have its function named mult along with keeping multiply and eventually deprecate the latter. But probably that belongs in a different issue/PR

[ksen0]

Related fix has been merged