Skip to content

Fix mesh instance sort key when using drawBucket #7819

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

Merged
merged 1 commit into from
Jul 4, 2025

Conversation

mvaligursky
Copy link
Contributor

  • we need 8 bits for the drawBucket, but only gave it 6 bits, which would cause incorrect sorting in some cases

@mvaligursky mvaligursky self-assigned this Jul 4, 2025
@mvaligursky mvaligursky added bug area: graphics Graphics related issue labels Jul 4, 2025
@mvaligursky mvaligursky requested review from Copilot and a team July 4, 2025 10:25
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR corrects the bit allocation for the draw bucket in the mesh instance sort key, expanding it from 6 bits to 8 bits to avoid sorting errors.

  • Adjusted comment to reflect new 8-bit draw bucket range.
  • Updated bit shifts and masks in updateKey() to use 8 bits for draw bucket and 22 bits for material ID.
  • Shifted alpha test bit accordingly.
Comments suppressed due to low confidence (1)

src/scene/mesh-instance.js:1086

  • Add unit tests for updateKey() that cover drawBucket values above 63 to verify the new 8-bit allocation and ensure correct sort ordering.
    updateKey() {

@mvaligursky mvaligursky merged commit 59f77ff into main Jul 4, 2025
7 checks passed
@mvaligursky mvaligursky deleted the mv-drawbucket-key-fix branch July 4, 2025 10:27
mvaligursky added a commit that referenced this pull request Jul 4, 2025
@kungfooman
Copy link
Collaborator

Hm, I used this code before to have depthTest without complicating everything with layers for a prototype:

        const material = new pc.StandardMaterial({diffuse: pc.Color.RED});
        const test = Math.random() > 0.5;
        if (test) {
          console.log("Setting depthTest to false");
          material.depthTest = false;
          //material.depthFunc = pc.FUNC_ALWAYS;
          material.update();
        }
        const marker = new pc.Entity("marker");
        marker.addComponent('model', {type: 'box', material});
        marker.setPosition(intersection);
        marker.setLocalScale(0.1, 0.1, 0.1);
        if (test) {
          marker.model.meshInstances[0].drawBucket = 254;
        }
        this.app.root.addChild(marker);

But now this trick doesn't work any longer, do you happen to know what's wrong?

@mvaligursky
Copy link
Contributor Author

are you saying this PR makes it not working?

@kungfooman
Copy link
Collaborator

kungfooman commented Jul 5, 2025

are you saying this PR makes it not working?

yep, I just tested more and it does work when I make it lower than the default 127 value - so when I set it to 126, I can prioritize the mesh instance to make material.depthTest = false; work - but this is upside-down now from what it was?

@kungfooman
Copy link
Collaborator

Mini cube-in-cube example where the inner cube as deactivated depth testing:

<!DOCTYPE html>
<html>
<head>
  <title>PlayCanvas Two Cubes</title>
  <meta charset="utf-8">
  <style>
    body {margin: 0;}
    canvas {width: 100%;height: 100%;display: block;}
  </style>
  <script type="importmap">{"imports": {
    "playcanvas": "/playcanvas-engine-clean/src/index.js",
    "fflate": "/playcanvas-engine-clean/node_modules/fflate/esm/browser.js",
    "playcanvas/scripts/": "https://cdn.jsdelivr.net/npm/[email protected]/scripts/esm/"
  }}</script>
</head>
<body>
  <canvas id="application-canvas"></canvas>
  <script type="module">
    import * as pc from 'playcanvas';
    const canvas = document.getElementById('application-canvas');
    const app = new pc.Application(canvas);
    app.setCanvasFillMode(pc.FILLMODE_FILL_WINDOW);
    app.setCanvasResolution(pc.RESOLUTION_AUTO);
    window.addEventListener('resize', () => app.resizeCanvas());
    // Set up the camera
    const camera = new pc.Entity('camera');
    camera.addComponent('camera', {clearColor: new pc.Color(0, 0, 0.5)});
    camera.setPosition(0, 0, 5);
    app.root.addChild(camera);
    // Create outer cube
    const outerCube = new pc.Entity('outerCube');
    outerCube.addComponent('render', {type: 'box'});
    outerCube.setLocalScale(2, 2, 2);
    app.root.addChild(outerCube);
    // Create inner cube with depth testing disabled
    const innerCube = new pc.Entity('innerCube');
    const innerMaterial = new pc.StandardMaterial();
    innerMaterial.diffuse = new pc.Color(1, 0, 0); // Red to distinguish
    innerMaterial.depthTest = false;
    innerMaterial.update();
    innerCube.addComponent('render', {type: 'box', material: innerMaterial});
    innerCube.render.meshInstances[0].drawBucket = 126;
    app.root.addChild(innerCube);
    // Random rotation speeds and directions
    const outerRotationSpeed = new pc.Vec3(Math.random() - 0.5, Math.random() - 0.5, Math.random() - 0.5).scale(50);
    const innerRotationSpeed = new pc.Vec3(Math.random() - 0.5, Math.random() - 0.5, Math.random() - 0.5).scale(50);
    // Update loop for rotation
    app.on('update', (dt) => {
      outerCube.rotate(outerRotationSpeed.x * dt, outerRotationSpeed.y * dt, outerRotationSpeed.z * dt);
      innerCube.rotate(innerRotationSpeed.x * dt, innerRotationSpeed.y * dt, innerRotationSpeed.z * dt);
    });
    app.scene.ambientLight = new pc.Color(0.75, 0.75, 0.75);
    app.start();
  </script>
</body>
</html>

Ideally it should look like:

image

Interesting line required now:

innerCube.render.meshInstances[0].drawBucket = 126;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants