Proposed fix for issue #613

May I commit the following patch to /trunk/engine/src/core/com/jme3/shadow/AbstractShadowRenderer.java and also to /branches/gradle-restructure/jme3-core/src/main/java/com/jme3/shadow/AbstractShadowRenderer.java ?

--- Base (BASE)
+++ Locally Modified (Based On LOCAL)
@@ -122,7 +122,7 @@
      * @param shadowMapSize the size of the rendered shadowmaps (512,1024,2048,
      * etc...)
      * @param nbShadowMaps the number of shadow maps rendered (the more shadow
-     * maps the more quality, the less fps).
+     * maps the more quality, the fewer fps).
      */
     protected AbstractShadowRenderer(AssetManager assetManager, int shadowMapSize, int nbShadowMaps) {
 
@@ -192,7 +192,7 @@
      * Sets the filtering mode for shadow edges see {@link EdgeFilteringMode}
      * for more info
      *
-     * @param EdgeFilteringMode
+     * @param filterMode the desired filter mode
      */
     final public void setEdgeFilteringMode(EdgeFilteringMode filterMode) {
         if (filterMode == null) {
@@ -312,7 +312,7 @@
     }
 
     /**
-     * This mehtod is called once per frame. it is responsible for updating the
+     * This method is called once per frame. it is responsible for updating the
      * shadow cams according to the light view.
      *
      * @param viewCam the scene cam
@@ -320,12 +320,13 @@
     protected abstract void updateShadowCams(Camera viewCam);
 
     /**
-     * this method must return the geomtryList that contains the oclluders to be
+     * this method must return the geomtryList that contains the occluders to be
      * rendered in the shadow map
      *
      * @param shadowMapIndex the index of the shadow map being rendered
      * @param sceneOccluders the occluders of the whole scene
-     * @param sceneReceivers the recievers of the whole scene
+     * @param sceneReceivers the receivers of the whole scene
+     * @param shadowMapOcculders
      * @return
      */
     protected abstract GeometryList getOccludersToRender(int shadowMapIndex, GeometryList sceneOccluders, GeometryList sceneReceivers, GeometryList shadowMapOccluders);
@@ -421,8 +422,8 @@
     }
 
     /**
-     * For dubuging purpose Allow to "snapshot" the current frustrum to the
-     * scene
+     * For debugging purposes, "snapshot" the current frustum to the
+     * scene.
      */
     public void displayDebug() {
         debug = true;
@@ -472,7 +473,7 @@
      * This method is called once per frame and is responsible of setting the
      * material parameters than sub class may need to set on the post material
      *
-     * @param material the materail to use for the post shadow pass
+     * @param material which material to use for the post shadow pass
      */
     protected abstract void setMaterialParameters(Material material);
 
@@ -543,7 +544,7 @@
     }
 
     /**
-     * returns the shdaow intensity
+     * Read the shadow intensity.
      *
      * @see #setShadowIntensity(float shadowIntensity)
      * @return shadowIntensity
@@ -554,7 +555,7 @@
 
     /**
      * Set the shadowIntensity, the value should be between 0 and 1, a 0 value
-     * gives a bright and invisilble shadow, a 1 value gives a pitch black
+     * gives a bright and invisible shadow, a 1 value gives a pitch black
      * shadow, default is 0.7
      *
      * @param shadowIntensity the darkness of the shadow
@@ -606,11 +607,16 @@
         this.flushQueues = flushQueues;
     }
 
+    /**
+     * De-serialize this instance when loading from a J3O file.
+     *
+     * @param im importer (not null)
+     */
     public void read(JmeImporter im) throws IOException {
         InputCapsule ic = (InputCapsule) im.getCapsule(this);
         assetManager = im.getAssetManager();
         nbShadowMaps = ic.readInt("nbShadowMaps", 1);
-        shadowMapSize = ic.readInt("shadowMapSize", 0);
+        shadowMapSize = ic.readFloat("shadowMapSize", 0f);
         shadowIntensity = ic.readFloat("shadowIntensity", 0.7f);
         edgeFilteringMode = ic.readEnum("edgeFilteringMode", EdgeFilteringMode.class, EdgeFilteringMode.Bilinear);
         shadowCompareMode = ic.readEnum("shadowCompareMode", CompareMode.class, CompareMode.Hardware);
@@ -621,6 +627,11 @@
 
     }
 
+    /**
+     * Serialize this instance when saving to a J3O file.
+     *
+     * @param ex exporter (not null)
+     */
\ No newline at end of file
     public void write(JmeExporter ex) throws IOException {
         OutputCapsule oc = (OutputCapsule) ex.getCapsule(this);
         oc.write(nbShadowMaps, "nbShadowMaps", 1);

Most of it looks good to me. One type missed: geomtryList

I will let others speak to whether this breaks older j3os:
shadowMapSize = ic.readFloat(ā€œshadowMapSizeā€, 0f);

Iā€™m not really sure why shadowMapSize is a float to begin withā€¦ and the write() method is still writing an int. Edit: never mind on this one.

Also, I donā€™t like this part:

  • * @param material the materail to use for the post shadow pass
    
  • * @param material which material to use for the post shadow pass
    

The first one is the normal convention and the one that all of the other parameters even in this file are following.

P.S.: Itā€™s super cool that you are going through these. If you think of it and itā€™s not too much trouble, Iā€™m wondering if you could include the issue link in these posts in the future. Would have saved me a bunch of time on this one.

Actually, Iā€™d almost say that doc fixes and bug fixes should be separate commits because I almost missed the actual functional changes in the original skim. (Except in cases where the doc changes are directly related to the code changes.)

A note as to what was fixed might be a good clue, too.

Okay, Iā€™ll split this into two commits. Hereā€™s the diff to fix the bug:

--- Base (BASE)
+++ Locally Modified (Based On LOCAL)
@@ -610,7 +610,7 @@
         InputCapsule ic = (InputCapsule) im.getCapsule(this);
         assetManager = im.getAssetManager();
         nbShadowMaps = ic.readInt("nbShadowMaps", 1);
-        shadowMapSize = ic.readInt("shadowMapSize", 0);
+        shadowMapSize = ic.readFloat("shadowMapSize", 0f);
         shadowIntensity = ic.readFloat("shadowIntensity", 0.7f);
         edgeFilteringMode = ic.readEnum("edgeFilteringMode", EdgeFilteringMode.class, EdgeFilteringMode.Bilinear);
         shadowCompareMode = ic.readEnum("shadowCompareMode", CompareMode.class, CompareMode.Hardware);

Hereā€™s the link to the issue: https://code.google.com/p/jmonkeyengine/issues/detail?id=613

Is this OK to commit?

Yep. Looks ok to me.

@pspeed said:

Also, I donā€™t like this part:

  • * @param material the materail to use for the post shadow pass
    
  • * @param material which material to use for the post shadow pass
    

The first one is the normal convention and the one that all of the other parameters even in this file are following.

I didnā€™t get why you objected to the spell correction of ā€˜materialā€™?

You donā€™t like my french spelling?
materail, geomtry, recieverā€¦

About the bug fix, youā€™re right when you say that the size should be an int. I fail to remember the reason why I used a floatā€¦but I think there was oneā€¦For now your fix will be ok.
Thanks, go ahead and commit

1 Like
@jmaasing said: I didn't get why you objected to the spell correction of 'material'?

I didnā€™t notice that. Itā€™s the superfluous ā€œwhichā€ that I had a problem with.

@nehon said: You don't like my french spelling? materail, geomtry, reciever....

About the bug fix, youā€™re right when you say that the size should be an int. I fail to remember the reason why I used a floatā€¦but I think there was oneā€¦For now your fix will be ok.
Thanks, go ahead and commit

Your English is far better than my French, so I dare not complain. :slight_smile:

Iā€™ve committed. Would someone please close the issue at Google Code?

Itā€™s 2 am here. Iā€™ll propose new comment diffs after I get some sleep.

pspeed said: I didn't notice that. It's the superfluous "which" that I had a problem with.

And I, not being a native speaker, missed the ā€˜whichā€™ :roll: proof-reading is hard

cough Not to get totally English up in this mofo should it not read

@param material is the material to be used for the post shadow pass

Just throwing that out there.

@thecyberbob said: *cough* Not to get totally English up in this mofo should it not read

Just throwing that out there.

No, because this is javadoc and will be parsed and formatted and so it wonā€™t read just like one sentence anymore.

@pspeed Hokee dokee. Just seemed like a strange phrase to me. :slight_smile:

@thecyberbob said: @pspeed Hokee dokee. Just seemed like a strange phrase to me. :)

Well, itā€™s the difference between:
material - The material to be used for the post shadow pass

And:
material - Is the material to be used for the post shadow pass

ā€¦because thatā€™s how it will be formatted in the standard javadoc and the second one sounds like it was spoken by someone for whom English is a second or third language.

The format for an @param tag is:
@param [name] [description]

It isnā€™t:
@param [a sentence about a parameter]

Ah. That makes more sense. In any case going from this proposed change

@param material which material to use for the post shadow pass

Should be

@param material The material to be used for the post shadow pass

Anyways I shall go back to my usual forum lurking. :oops:

@thecyberbob said: Ah. That makes more sense. In any case going from this proposed change

@param material which material to use for the post shadow pass

Should be

@param material The material to be used for the post shadow pass

Anyways I shall go back to my usual forum lurking. :oops:

javadoc will capitalize the first letter of the description if necessary.

Hereā€™s a new set of proposed changes, consisting entirely of comments. The affected files would be /trunk/engine/src/core/com/jme3/shadow/AbstractShadowRenderer.java and also to /branches/gradle-restructure/jme3-core/src/main/java/com/jme3/shadow/AbstractShadowRenderer.java

May I commit these?

--- Base (BASE)
+++ Locally Modified (Based On LOCAL)
@@ -92,19 +92,26 @@
     protected CompareMode shadowCompareMode = CompareMode.Hardware;
     protected Picture[] dispPic;
     protected boolean flushQueues = true;
-    // define if the fallback material should be used.
+    /**
+     * true if the fallback material should be used, otherwise false
+     */
     protected boolean needsfallBackMaterial = false;
-    //Name of the post material technique
+    /**
+     * name of the post material technique
+     */
     protected String postTechniqueName = "PostShadow";
-    //flags to know when to change params in the materials
-    //a list of material of the post shadow queue geometries.
+    /**
+     * list of materials for post shadow queue geometries
+     */
     protected List<Material> matCache = new ArrayList<Material>();
     protected GeometryList sceneReceivers;
     protected GeometryList lightReceivers = new GeometryList(new OpaqueComparator());
     protected GeometryList shadowMapOccluders = new GeometryList(new OpaqueComparator());
     private String[] shadowMapStringCache;
     private String[] lightViewStringCache;
-    //used to skip the post pass when there are no shadow casters.
+    /**
+     * true to skip the post pass when there are no shadow casters
+     */
     protected boolean skipPostPass;
 
     
@@ -115,14 +122,14 @@
     }    
     
     /**
-     * Create an abstract shadow renderer, this is to be called in extending
-     * classes
+     * Create an abstract shadow renderer. Extending classes should invoke this
+     * constructor.
      *
      * @param assetManager the application asset manager
-     * @param shadowMapSize the size of the rendered shadowmaps (512,1024,2048,
+     * @param shadowMapSize the size of the rendered shadow maps (512,1024,2048,
      * etc...)
      * @param nbShadowMaps the number of shadow maps rendered (the more shadow
-     * maps the more quality, the less fps).
+     * maps the more quality, the fewer fps).
      */
     protected AbstractShadowRenderer(AssetManager assetManager, int shadowMapSize, int nbShadowMaps) {
 
@@ -189,10 +196,10 @@
     }
 
     /**
-     * Sets the filtering mode for shadow edges see {@link EdgeFilteringMode}
-     * for more info
+     * Sets the filtering mode for shadow edges. See {@link EdgeFilteringMode}
+     * for more info.
      *
-     * @param EdgeFilteringMode
+     * @param filterMode the desired filter mode (not null)
      */
     final public void setEdgeFilteringMode(EdgeFilteringMode filterMode) {
         if (filterMode == null) {
@@ -216,7 +223,7 @@
     }
 
     /**
-     * returns the the edge filtering mode
+     * returns the edge filtering mode
      *
      * @see EdgeFilteringMode
      * @return
@@ -226,9 +233,9 @@
     }
 
     /**
-     * sets the shadow compare mode see {@link CompareMode} for more info
+     * Sets the shadow compare mode. See {@link CompareMode} for more info.
      *
-     * @param compareMode
+     * @param compareMode the desired compare mode (not null)
      */
     final public void setShadowCompareMode(CompareMode compareMode) {
         if (compareMode == null) {
@@ -265,7 +272,9 @@
         return shadowCompareMode;
     }
 
-    //debug function that create a displayable frustrum
+    /**
+     * debug function to create a visible frustum
+     */
     protected Geometry createFrustum(Vector3f[] pts, int i) {
         WireFrustum frustum = new WireFrustum(pts);
         Geometry frustumMdl = new Geometry("f", frustum);
@@ -296,6 +305,12 @@
         return frustumMdl;
     }
 
+    /**
+     * Initialize this shadow renderer prior to its first update.
+     *
+     * @param rm the render manager
+     * @param vp the viewport
+     */
     public void initialize(RenderManager rm, ViewPort vp) {
         renderManager = rm;
         viewPort = vp;
@@ -307,12 +322,17 @@
         }
     }
 
+    /**
+     * Test whether this shadow renderer has been initialized.
+     *
+     * @return true if initialized, otherwise false
+     */
     public boolean isInitialized() {
         return viewPort != null;
     }
 
     /**
-     * This mehtod is called once per frame. it is responsible for updating the
+     * This method is called once per frame. it is responsible for updating the
      * shadow cams according to the light view.
      *
      * @param viewCam the scene cam
@@ -320,12 +340,13 @@
     protected abstract void updateShadowCams(Camera viewCam);
 
     /**
-     * this method must return the geomtryList that contains the oclluders to be
-     * rendered in the shadow map
+     * return a geometryList that contains the occluders to be rendered in the
+     * shadow map
      *
      * @param shadowMapIndex the index of the shadow map being rendered
      * @param sceneOccluders the occluders of the whole scene
-     * @param sceneReceivers the recievers of the whole scene
+     * @param sceneReceivers the receivers of the whole scene
+     * @param shadowMapOcculders
      * @return
      */
     protected abstract GeometryList getOccludersToRender(int shadowMapIndex, GeometryList sceneOccluders, GeometryList sceneReceivers, GeometryList shadowMapOccluders);
@@ -421,8 +442,7 @@
     }
 
     /**
-     * For dubuging purpose Allow to "snapshot" the current frustrum to the
-     * scene
+     * For debugging purposes, "snapshot" the current frustum to the scene.
      */
     public void displayDebug() {
         debug = true;
@@ -469,10 +489,10 @@
     }
 
     /**
-     * This method is called once per frame and is responsible of setting the
-     * material parameters than sub class may need to set on the post material
+     * This method is called once per frame and is responsible for setting any
+     * material parameters than subclass may need to set on the post material.
      *
-     * @param material the materail to use for the post shadow pass
+     * @param material the material to use for the post shadow pass
      */
     protected abstract void setMaterialParameters(Material material);
 
@@ -543,7 +563,7 @@
     }
 
     /**
-     * returns the shdaow intensity
+     * Read the shadow intensity.
      *
      * @see #setShadowIntensity(float shadowIntensity)
      * @return shadowIntensity
@@ -553,9 +573,9 @@
     }
 
     /**
-     * Set the shadowIntensity, the value should be between 0 and 1, a 0 value
-     * gives a bright and invisilble shadow, a 1 value gives a pitch black
-     * shadow, default is 0.7
+     * Set the shadowIntensity. The value should be between 0 and 1. A 0 value
+     * gives a bright and invisible shadow, a 1 value gives a pitch black
+     * shadow. The default is 0.7
      *
      * @param shadowIntensity the darkness of the shadow
      */
@@ -587,7 +607,7 @@
     }
 
     /**
-     * returns true if the PssmRenderer flushed the shadow queues
+     * returns true if the shadow renderer flushes the shadow queues
      *
      * @return flushQueues
      */
@@ -596,9 +616,9 @@
     }
 
     /**
-     * Set this to false if you want to use several PssmRederers to have
-     * multiple shadows cast by multiple light sources. Make sure the last
-     * PssmRenderer in the stack DO flush the queues, but not the others
+     * Set flushQueues to false if you have multiple shadow renderers, in order
+     * for multiple light sources to cast shadows. Make sure the last shadow
+     * renderer in the stack DOES flush the queues, but not the others.
      *
      * @param flushQueues
      */
@@ -606,6 +626,11 @@
         this.flushQueues = flushQueues;
     }
 
+    /**
+     * De-serialize this instance when loading from a J3O file.
+     *
+     * @param im importer (not null)
+     */
     public void read(JmeImporter im) throws IOException {
         InputCapsule ic = (InputCapsule) im.getCapsule(this);
         assetManager = im.getAssetManager();
@@ -621,6 +646,11 @@
 
     }
 
+    /**
+     * Serialize this instance when saving to a J3O file.
+     *
+     * @param ex exporter (not null)
+     */
\ No newline at end of file
     public void write(JmeExporter ex) throws IOException {
         OutputCapsule oc = (OutputCapsule) ex.getCapsule(this);
         oc.write(nbShadowMaps, "nbShadowMaps", 1);
1 Like
@sgold said: Here's a new set of proposed changes, consisting entirely of comments. The affected files would be /trunk/engine/src/core/com/jme3/shadow/AbstractShadowRenderer.java and also to /branches/gradle-restructure/jme3-core/src/main/java/com/jme3/shadow/AbstractShadowRenderer.java

May I commit these?

Looks good, but read/write is not only for j3o files, actually in this case its mainly j3f files. Generally it can be used for any kind of serialization, e.g. I use it for the clipboard in the SDK. Maybe add an ā€œā€¦, e.g. whenā€¦ā€

1 Like

Most of it looks goodā€¦ I have a few comments. I know it will sound overly pedantic but getting comments right is sort of a pedantic exercise on its own.

First:

  • * Create an abstract shadow renderer, this is to be called in extending
    
  • * classes
    
  • * Create an abstract shadow renderer. Extending classes should invoke this
    
  • * constructor.
    

Technically, the old one was more correct in a subtle wayā€¦ though neither are technically right. Because itā€™s a protected constructor on an abstract class nothing will ever be calling this except the subclassā€¦ and it doesnā€™t have much of a choice about it. I would just change it to ā€œSubclasses invoked this constructorā€ or something. There is one constructor for serialization and one for subclasses to initialize the base class.

  • * This mehtod is called once per frame. it is responsible for updating the
    
  • * This method is called once per frame. it is responsible for updating the
    

I think the second sentence should be capitalized but I would combine it all into one sentence anyway. When browsing javadoc itā€™s better to put as much useful info into the first sentence as possible since that will be the one in the method index. So perhaps:
ā€œCalled once per frame to update the shadow cams according to the light view.ā€

  • * this method must return the geomtryList that contains the oclluders to be
    
  • * rendered in the shadow map
    
  • * return a geometryList that contains the occluders to be rendered in the
    
  • * shadow map
    

ā€œReturns a subclass specific geometryList containing the occluders to be rendered in the shadow map.ā€

  • * returns the shdaow intensity
    
  • * Read the shadow intensity.
    

ā€œReturns the shadow intensity.ā€

  • * returns true if the PssmRenderer flushed the shadow queues
    
  • * returns true if the shadow renderer flushes the shadow queues
    

ā€œReturns true if the shadow renderer flushes the shadow queues.ā€

Those were what I saw at a quick glance through the patch. I might not have mentioned the lasts ones but I was already commenting.

@normen said: Looks good, but read/write is not only for j3o files, actually in this case its mainly j3f files. Generally it can be used for any kind of serialization, e.g. I use it for the clipboard in the SDK. Maybe add an "..., e.g. when.."

Thatā€™s good to know. Iā€™ll change this as you suggest, incorporate Paulā€™s suggestions, and come back with a revised diff after dinner.

Thanks, guys!