Skip to content

Conversation

@yaRnMcDonuts
Copy link
Member

@yaRnMcDonuts yaRnMcDonuts commented May 10, 2025

This PR adds a new test to jme-examples that reproduces the error reported in #2435 from using pbr with instancing.

I also updated PBRLighting.vert and fixed the wPosition calculation to solve the original issue.

@yaRnMcDonuts yaRnMcDonuts added this to the v3.9.0 milestone May 10, 2025
@yaRnMcDonuts
Copy link
Member Author

I plan to merge this within the next 2 days so that I can put out a 3.8.1 patch release to address this issue as soon as possible.

@yaRnMcDonuts yaRnMcDonuts added bug Something that is supposed to work, but doesn't. More severe than a "defect". examples specific to the jme3-examples sub-project labels May 10, 2025
@capdevon
Copy link
Contributor

capdevon commented May 10, 2025

Hi @yaRnMcDonuts,
thanks for fixing the issue! Could you please modify the test class like this to better highlight the components being tested in the context of resolving issue #2435?

e.g.

import java.util.Locale;

import com.jme3.app.SimpleApplication;
import com.jme3.font.BitmapText;
import com.jme3.light.DirectionalLight;
import com.jme3.material.Material;
import com.jme3.math.ColorRGBA;
import com.jme3.math.Vector3f;
import com.jme3.scene.Geometry;
import com.jme3.scene.instancing.InstancedNode;
import com.jme3.scene.shape.Box;

/**
 * This test specifically validates the corrected PBR rendering when combined
 * with instancing, as addressed in issue #2435. 
 * 
 * It creates an InstancedNode
 * with a PBR-materialized Box to ensure the fix in PBRLighting.vert correctly
 * handles world position calculations for instanced geometry.
 */
public class TestInstanceNodeWithPbr extends SimpleApplication {

    public static void main(String[] args) {
        TestInstanceNodeWithPbr app = new TestInstanceNodeWithPbr();
        app.start();
    }

    private BitmapText bmp;
    private Geometry box;
    private float pos = -5;
    private float vel = 5;
    
    @Override
    public void simpleInitApp() {
        configureCamera();
        bmp = createLabelText(10, 20, "<placeholder>");
        
        InstancedNode instancedNode = new InstancedNode("InstancedNode");
        rootNode.attachChild(instancedNode);

        Box mesh = new Box(0.5f, 0.5f, 0.5f);
        box = new Geometry("Box", mesh);
        Material pbrMaterial = createPbrMaterial(ColorRGBA.Red);
        box.setMaterial(pbrMaterial);

        instancedNode.attachChild(box);
        instancedNode.instance();

        DirectionalLight light = new DirectionalLight();
        light.setDirection(new Vector3f(-1, -2, -3).normalizeLocal());
        rootNode.addLight(light);
    }

    private Material createPbrMaterial(ColorRGBA color) {
        Material mat = new Material(assetManager, "Common/MatDefs/Light/PBRLighting.j3md");
        mat.setColor("BaseColor", color);
        mat.setFloat("Roughness", 0.8f);
        mat.setFloat("Metallic", 0.1f);
        mat.setBoolean("UseInstancing", true);
        return mat;
    }
    
    private void configureCamera() {
        flyCam.setMoveSpeed(15f);
        flyCam.setDragToRotate(true);

        cam.setLocation(Vector3f.UNIT_XYZ.mult(12));
        cam.lookAt(Vector3f.ZERO, Vector3f.UNIT_Y);
    }
    
    private BitmapText createLabelText(int x, int y, String text) {
        BitmapText bmp = new BitmapText(guiFont);
        bmp.setText(text);
        bmp.setLocalTranslation(x, settings.getHeight() - y, 0);
        bmp.setColor(ColorRGBA.Red);
        guiNode.attachChild(bmp);
        return bmp;
    }
    
    @Override
    public void simpleUpdate(float tpf) {
        pos += tpf * vel;
        if (pos < -10f || pos > 10f) {
            vel *= -1;
        }
        box.setLocalTranslation(pos, 0f, 0f);
        bmp.setText(String.format(Locale.ENGLISH, "BoxPosition: (%.2f, %.1f, %.1f)", pos, 0f, 0f));
    }

}

image

@yaRnMcDonuts
Copy link
Member Author

I've updated the test with your suggested changes. Thanks for the review!

@yaRnMcDonuts yaRnMcDonuts merged commit 2ca7b52 into master May 15, 2025
16 checks passed
@yaRnMcDonuts yaRnMcDonuts deleted the yaRnMcDonuts-TestInstanceNodeWithPbr branch May 15, 2025 20:36
@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented May 15, 2025

@capdevon I accidentally forgot to cherry-pick the 3rd commit in this PR when creating the 3.8.1-stable version yesterday. I cherry picked the first 2 commits I made, but I accidentally overlooked my 3rd commit where I made changes based on your review.

So those javadoc and formatting changes that you suggested will unfortunately not be in this new test in 3.8.1-stable, but they are in master and thus will be included in every subsequent release after 3.8.1-stable so everything should be alright.

richardTingle added a commit to richardTingle/jmonkeyengine that referenced this pull request May 16, 2025
richardTingle added a commit to richardTingle/jmonkeyengine that referenced this pull request May 16, 2025
richardTingle added a commit to richardTingle/jmonkeyengine that referenced this pull request May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something that is supposed to work, but doesn't. More severe than a "defect". examples specific to the jme3-examples sub-project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Need to create Tests using Instancing with PBR Updated pbr breaks instancing

3 participants