NotificationsYou must be signed in to change notification settings
Fork3.7k
Star23.3k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

davepagurek merged 3 commits intoprocessing:dev-2.0fromAbhayaj247:fix/atan-strands

Sep 18, 2025

Merged

Fix: allow single-arg atan() outside strands; add unit test#8096

davepagurek merged 3 commits intoprocessing:dev-2.0fromAbhayaj247:fix/atan-strands

Sep 18, 2025

Conversation

Copy link

Abhayaj247 commentedSep 17, 2025•
edited
Loading

Resolves#8092

The single-argumentatan(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 inFix noise() getting overridden; add tests #8036.
The fix marks the 1-arg overload as a normal p5 function while keeping the 2-arg GLSL overloadatan(y, x) strands-only.

Changes :

src/webgl/ShaderGenerator.js
- InbuiltInGLSLFunctions, set:
  - atan single-arg overload:isp5Function: true
  - atan two-arg overload: remainsisp5Function: false
test/unit/visual/cases/math.js
- Addatan_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
- Include the new visual test suite entryvisual/cases/math.

Screenshots of the change :

Visual test creates baseline showing text like “atan(0.5) ≈ 0.464”; subsequent runs match the baseline.

PR Checklist :

npm run lint passes
Inline reference is included / updated (function behavior unchanged; no reference edits needed)
Unit tests are included / updated (added visual/integration test)

Fix: allow single-arg atan() outside strands; add visual test

4a11290

davepagurek reviewed

Sep 17, 2025

View reviewed changes

Copy link

Contributor

davepagurek left a 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 withexpect(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?

Fix atan unit tests: handle single-arg and two-arg cases, remove visu…

b5021f5

…al math test

Copy link

Author

Abhayaj247 commentedSep 17, 2025•
edited
Loading

Thanks for the feedback@davepagurek , I’ve moved the atan tests to a dedicated unit test file attest/unit/math/atan.js . The tests now verify correct numerical results for single-argument usage and ensure the two-argument (GLSL-only) case returnsundefined outside of strands. This keeps the tests efficient, focused, and aligned with the intended testing strategy - helping maintain clarity in the suite.

Please let me know if you need any further adjustments.

Abhayaj247 requested a review fromdavepagurek

September 17, 2025 15:52

davepagurek reviewed

Sep 17, 2025

View reviewed changes

test/unit/math/atan.js Outdated

		const originalAtan = mockP5Prototype.atan;

		// Override atan to handle one-arg and two-arg correctly
		mockP5Prototype.atan = function(...args) {

Copy link

Contributor

davepagurekSep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can we test the actualatan without testing a mock version? I think the only thing we really need a test for is just that, outside of strands, you can callatan with one argument, which it already supports without needing an override.

Copy link

Contributor

davepagurekSep 17, 2025

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.

Copy link

Author

Abhayaj247Sep 17, 2025

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!

Copy link

Contributor

davepagurekSep 18, 2025

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.

Copy link

Author

Abhayaj247Sep 18, 2025

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.

Copy link

Contributor

davepagurekSep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

looks good!

Copy link

Author

Abhayaj247Sep 18, 2025

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.

Update atan unit tests to comply with maintainer feedback

b2e4e34

Abhayaj247 changed the title~~Fix: allow single-arg atan() outside strands; add visual test~~Fix: allow single-arg atan() outside strands; add unit test

Sep 18, 2025

davepagurek approved these changes

Sep 18, 2025

View reviewed changes

Copy link

Contributor

davepagurek left a 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!

davepagurek merged commitf03d3b5 intoprocessing:dev-2.0

Sep 18, 2025

2 checks passed

Copy link

Author

Abhayaj247 commentedSep 18, 2025

Thank you for your guidance throughout this process. I look forward to contributing more.

Labels

None yet

Movatterモバイル変換

Uh oh!

Fix: allow single-arg atan() outside strands; add unit test#8096

Fix: allow single-arg atan() outside strands; add unit test#8096

Uh oh!

Conversation

Abhayaj247 commentedSep 17, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Abhayaj247 commentedSep 17, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

davepagurekSep 17, 2025

Choose a reason for hiding this comment

Uh oh!

davepagurekSep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Abhayaj247Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

davepagurekSep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Abhayaj247Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

davepagurekSep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Abhayaj247Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Abhayaj247 commentedSep 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Abhayaj247 commentedSep 17, 2025•
edited
Loading

Abhayaj247 commentedSep 17, 2025•
edited
Loading