-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix: allow single-arg atan() outside strands; add unit test #8096
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this! Since the atan test is just checking the value of the result, I think it makes more sense to do a regular unit test rather than a visual test (it's a lot computationally cheaper to compare numbers with expect(val).toEqual(...)
than to compare all the pixels in an image, do you think we could put the test in one of our unit test files instead?
Thanks for the feedback @davepagurek , I’ve moved the atan tests to a dedicated unit test file at Please let me know if you need any further adjustments. |
test/unit/math/atan.js
Outdated
const originalAtan = mockP5Prototype.atan; | ||
|
||
// Override atan to handle one-arg and two-arg correctly | ||
mockP5Prototype.atan = function(...args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test the actual atan
without testing a mock version? I think the only thing we really need a test for is just that, outside of strands, you can call atan
with one argument, which it already supports without needing an override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason being, it becomes a bit unclear how much of the functionality we're testing is just in the mock once we start doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @davepagurek ,
I’ve updated the atan unit test to only verify single-argument behavior (in both radians and degrees) using the actual function, without overriding or mocking it.
import trigonometry from '../../../src/math/trigonometry.js';
import { assert } from 'chai';
suite('atan', function() {
const mockP5 = {
RADIANS: 'radians',
DEGREES: 'degrees',
_validateParameters: () => {}
};
const mockP5Prototype = {};
beforeEach(function() {
mockP5Prototype._angleMode = mockP5.RADIANS;
mockP5Prototype.angleMode = function(mode) {
this._angleMode = mode;
};
trigonometry(mockP5, mockP5Prototype);
});
test('should return the correct value for atan(0.5) in radians', function() {
mockP5Prototype.angleMode(mockP5.RADIANS);
const expected = Math.atan(0.5);
const actual = mockP5Prototype.atan(0.5);
assert.closeTo(actual, expected, 1e-10);
});
test('should return the correct value for atan(0.5) in degrees', function() {
mockP5Prototype.angleMode(mockP5.DEGREES);
const expected = Math.atan(0.5) * 180 / Math.PI;
const actual = mockP5Prototype.atan(0.5);
assert.closeTo(actual, expected, 1e-10);
});
});
Could you please confirm if this approach looks good before I commit and push the changes?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That approach looks right! maybe just evaluate the two functions on a calculator to get as specific of a value to compare to as possible for your radians and degrees cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification @davepagurek. I’ve updated the tests to directly compare against pre-calculated values for atan(0.5) in both radians and degrees. Here’s the full updated test file :
import trigonometry from '../../../src/math/trigonometry.js';
import { assert } from 'chai';
suite('atan', function() {
const mockP5 = {
RADIANS: 'radians',
DEGREES: 'degrees',
_validateParameters: () => {}
};
const mockP5Prototype = {};
beforeEach(function() {
mockP5Prototype._angleMode = mockP5.RADIANS;
mockP5Prototype.angleMode = function(mode) {
this._angleMode = mode;
};
trigonometry(mockP5, mockP5Prototype);
});
test('should return the correct value for atan(0.5) in radians', function() {
mockP5Prototype.angleMode(mockP5.RADIANS);
const expected = 0.4636476090008061; // pre-calculated value
const actual = mockP5Prototype.atan(0.5);
assert.closeTo(actual, expected, 1e-10);
});
test('should return the correct value for atan(0.5) in degrees', function() {
mockP5Prototype.angleMode(mockP5.DEGREES);
const expected = 26.56505117707799; // pre-calculated value
const actual = mockP5Prototype.atan(0.5);
assert.closeTo(actual, expected, 1e-10);
});
});
Could you confirm if this looks good before I push the changes? Happy to make any further adjustments if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing and confirming, @davepagurek! I’ve pushed the changes accordingly. Please let me know if there’s anything else needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR!
Thank you for your guidance throughout this process. I look forward to contributing more. |
Resolves #8092
atan(x)
was incorrectly treated as GLSL-only in strands, causing a friendly error and returning undefined when called outside a shader. This mirrors thenoise()
override issue fixed in Fix noise() getting overridden; add tests #8036.atan(y, x)
strands-only.Changes :
src/webgl/ShaderGenerator.js
builtInGLSLFunctions
, set:atan
single-arg overload:isp5Function: true
atan
two-arg overload: remainsisp5Function: false
test/unit/visual/cases/math.js
atan_outside_strands
visual integration test that draws the value ofatan(0.5)
to ensure it evaluates and no friendly error is triggered.test/unit/spec.js
visual/cases/math
.Screenshots of the change :
PR Checklist :