Skip to content

Refactor: AbstractShadowFilter reduce object allocations #2516

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor: AbstractShadowFilter reduce object allocations #2516

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 12 additions & 34 deletions jme3-core/src/main/java/com/jme3/shadow/AbstractShadowFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@
package com.jme3.shadow;

import com.jme3.asset.AssetManager;
import com.jme3.export.InputCapsule;
import com.jme3.export.JmeExporter;
import com.jme3.export.JmeImporter;
import com.jme3.export.OutputCapsule;
import com.jme3.material.Material;
import com.jme3.material.RenderState;
import com.jme3.math.Matrix4f;
Expand All @@ -48,35 +44,34 @@
import com.jme3.util.clone.Cloner;
import com.jme3.util.clone.JmeCloneable;

import java.io.IOException;

/**
* Generic abstract filter that holds common implementations for the different
* shadow filters
*
* @author Rémy Bouquet aka Nehon
* @author Nehon
Copy link
Member

Choose a reason for hiding this comment

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

Why have you changed the author?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original author's alias 'Nehon' is already the standard signature for most of their contributions, consistent with their GitHub nickname. To maintain uniformity with the other 500 classes in the jme3-core module and to align with this established practice, I've adjusted the signatures in these four Shadow classes. My intention is never to remove an original author's alias from any jME class, only to ensure consistency where a clear alias already exists and is widely used. This change also implicitly supports privacy by standardizing the alias used.

*/
public abstract class AbstractShadowFilter<T extends AbstractShadowRenderer> extends Filter implements Cloneable, JmeCloneable {
public abstract class AbstractShadowFilter<T extends AbstractShadowRenderer> extends Filter implements JmeCloneable {
Copy link
Member

Choose a reason for hiding this comment

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

Why remove Cloneable? (Why was it here in the first place?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The JmeCloneable interface already extends java.lang.Cloneable. Therefore, the explicit extension is redundant and its removal simplifies the code without altering its behavior. Therefore, what checks have you already performed that led you to raise this question?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes good point. No objection to removing it then


protected T shadowRenderer;
protected ViewPort viewPort;

private final Vector4f tempVec4 = new Vector4f();
private final Matrix4f tempMat4 = new Matrix4f();

/**
* For serialization only. Do not use.
*/
protected AbstractShadowFilter() {
}

/**
* Abstract class constructor
* Creates an AbstractShadowFilter. Subclasses invoke this constructor.
*
* @param manager the application asset manager
* @param shadowMapSize the size of the rendered shadowmaps (512,1024,2048,
* etc...)
* @param shadowRenderer the shadowRenderer to use for this Filter
* @param assetManager The application's asset manager.
* @param shadowMapSize The size of the rendered shadow maps (e.g., 512, 1024, 2048).
* @param shadowRenderer The shadowRenderer to use for this Filter
*/
@SuppressWarnings("all")
protected AbstractShadowFilter(AssetManager manager, int shadowMapSize, T shadowRenderer) {
protected AbstractShadowFilter(AssetManager assetManager, int shadowMapSize, T shadowRenderer) {
super("Post Shadow");
this.shadowRenderer = shadowRenderer;
// this is legacy setting for shadows with backface shadows
Expand All @@ -93,18 +88,12 @@ protected boolean isRequiresDepthTexture() {
return true;
}

public Material getShadowMaterial() {
return material;
}

Vector4f tmpv = new Vector4f();

@Override
protected void preFrame(float tpf) {
shadowRenderer.preFrame(tpf);
material.setMatrix4("ViewProjectionMatrixInverse", viewPort.getCamera().getViewProjectionMatrix().invert());
Matrix4f m = viewPort.getCamera().getViewProjectionMatrix();
material.setVector4("ViewProjectionMatrixRow2", tmpv.set(m.m20, m.m21, m.m22, m.m23));
material.setMatrix4("ViewProjectionMatrixInverse", tempMat4.set(m).invertLocal());
material.setVector4("ViewProjectionMatrixRow2", tempVec4.set(m.m20, m.m21, m.m22, m.m23));
}

@Override
Expand Down Expand Up @@ -337,15 +326,4 @@ public void cloneFields(final Cloner cloner, final Object original) {
shadowRenderer.setPostShadowMaterial(material);
}

@Override
public void write(JmeExporter ex) throws IOException {
super.write(ex);
OutputCapsule oc = ex.getCapsule(this);
}

@Override
public void read(JmeImporter im) throws IOException {
super.read(im);
InputCapsule ic = im.getCapsule(this);
}
}