Skip to content
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

cleanup: Remove VoxelPrimitive.jitter option (#11913) #12146

Merged
merged 3 commits into from
Aug 26, 2024
Merged

cleanup: Remove VoxelPrimitive.jitter option (#11913) #12146

merged 3 commits into from
Aug 26, 2024

Conversation

okrayum
Copy link
Contributor

@okrayum okrayum commented Aug 22, 2024

Description

In VoxelPrimitive.js I removed the following code blocks:

/**

/**

  • Gets or sets whether or not to jitter the view ray during the raymarch.

  • This reduces stair-step artifacts but introduces noise.

  • @memberof VoxelPrimitive.prototype

  • @type {boolean}
    */
    jitter: {
    get: function () {
    return this._jitter;
    },
    set: function (jitter) {
    //>>includeStart('debug', pragmas.debug);
    Check.typeOf.bool("jitter", jitter);
    //>>includeEnd('debug');

    if (this._jitter !== jitter) {
    this._jitter = jitter;
    this._shaderDirty = true;
    }
    },
    },

In VoxelRenderResources.js I removed the following code block:

if (primitive._jitter) {
shaderBuilder.addDefine("JITTER", undefined, ShaderDestination.FRAGMENT);
}

In VoxelPrimitiveSpec.js I removed the following line from the test "toggles render options that require shader rebuilds":

toggleOption("jitter", false, true);

This was a cleanup issue #11913

Issue number and link

Issue #11913

Testing plan

I ran npm run test prior to making any changes in the codebase and it showed 11 failing tests, I ran it again after making changes and it had the same failing tests. This was all done in the terminal, I then ran the tests through the browser, and it showed one failing test. I'm not sure if that is just in my environment. If they fail in the workflow I may need some additional guidance.

Author checklist

  • [ x] I have submitted a Contributor License Agreement
  • [ x] I have added my name to CONTRIBUTORS.md
  • [ x] I have updated CHANGES.md with a short summary of my change
  • [ x] I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • [ x] I have performed a self-review of my code

If approved this will close issue #11913

Copy link

Thank you for the pull request, @okrayum!

✅ We can confirm we have a CLA on file for you.

@okrayum
Copy link
Contributor Author

okrayum commented Aug 23, 2024

This is ready for workflow checks.

@ggetz
Copy link
Contributor

ggetz commented Aug 23, 2024

Thanks @okrayum! @ankittrehan2000 will be reviewing this PR.

Copy link
Contributor

@ankittrehan2000 ankittrehan2000 left a comment

Choose a reason for hiding this comment

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

@okrayum this looks good to merge, I just have the one comment. Thanks!

CHANGES.md Outdated Show resolved Hide resolved
@ankittrehan2000 ankittrehan2000 merged commit fd7eb81 into CesiumGS:main Aug 26, 2024
4 checks passed
@ankittrehan2000
Copy link
Contributor

Thanks @okrayum

@okrayum
Copy link
Contributor Author

okrayum commented Aug 26, 2024

@ankittrehan2000 you're welcome, happy I could help.

@okrayum okrayum deleted the voxel-jitter-cleanup branch August 26, 2024 19:20
@okrayum okrayum restored the voxel-jitter-cleanup branch August 27, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants