[COMMITTED] Some sphere improvements

archive contribution-depot-jme2
1

I have a couple changes to the sphere object I'd like to contribute.  The first adjusts the spacing between the latitudinal slices.  Currently these are spaced evenly along the sphere's Z axis, but this actually puts too many slices near the equator and not enough at the poles.  The result is a sphere with pointy poles.  The change spaces the slices evenly as measured along the surface of the sphere.  You can see the result below (both spheres use the same number of slices).


Current:
 New:


The second change adds a new texture mapping mode: TEX_POLAR.  This applies the texture straight down on the poles of the sphere rather than wrapping it around.  A major advantage to this is that eliminates the distortion one gets at the poles when using either TEX_ORIGINAL or TEX_PROJECTED.  A disadvantage however is that the texture mirrors across the equator.  Depending on the texture you use this may or may not be a big deal.  Here is the result (both images are of the sphere
2

Looks great. I'm all up to this.



Just a couple of points:


  1. the new spacing is a bit slower at construction… it should be optional and it should break existing behaviour if possible: either provide alternate constructors and methods, or implement it in a new class inheriting from Sphere.


  2. following the current style line, ideally, TEX_POLAR and TEX_PROJECTED should be an Enum, probably a member Enum.



    Thanks!
3

You're right. Speed surely won't be an issue.



About the default, I was more concerned because I think spheres are used to represent ModelBounds.



Arguably, as JME2 is not final, I think it's also correct to change the default behaviour and use your algorithm.



Adding documentation and allowing for both behaviours would be ideal  ;).

4

Nice patch.

Maybe default to the new way of uneven spaces slices (as it looks better), and a boolean in the spehre constructor boolean useEvenSlices for backwards compatibility?



I don't think you notice any speed difference just because of a added sin().

5

I have to admit I'm hard pressed to imagine a use case for switching back to the old model, but I suppose it doesn't hurt to make it an option – easy enough.  I agree that the speed difference should be minimal (I can measure it if there's concern.)



With respect to using enums – I completely agree and had considered switching the TEX_* variables over to an enum; however I was concerned about breaking API compatibility.  Figured it was better just to follow the existing convention.

6

Here's the new code based on your recommendations:


@@ -55,15 +55,30 @@

 
     private static final long serialVersionUID = 1L;
 
+    @Deprecated
     public static final int TEX_ORIGINAL = 0;
 
     // Spherical projection mode, donated by Ogli from the jME forums.
+    @Deprecated
     public static final int TEX_PROJECTED = 1;
+       
+    public enum TextureMode {
+        /** Wrap texture radially and along z-axis */
+        Original,
+        /** Wrap texure radially, but spherically project along z-axis */
+        Projected,
+        /** Apply texture to each pole.  Eliminates polar distortion,
+         * but mirrors the texture across the equator
+         */
+        Polar
+    }
 
     protected int zSamples;
 
     protected int radialSamples;
 
+    protected boolean useEvenSlices;
+
     /** the distance from the center point each point falls on */
     public float radius;
     /** the center of the sphere */
@@ -75,8 +90,7 @@

 
     private static Vector3f tempVc = new Vector3f();
 
-    // TODO: replace with a boolean property
-    protected int textureMode = TEX_ORIGINAL;
+    protected TextureMode textureMode = TextureMode.Original;
 
     public Sphere() {
     }
@@ -128,8 +142,29 @@

      */
     public Sphere(String name, Vector3f center, int zSamples,
             int radialSamples, float radius) {
+        this(name, center, zSamples, radialSamples, radius, false);
+    }
+
+    /**
+     * Constructs a sphere. Additional arg to evenly space latitudinal slices
+     *
+     * @param name
+     *            Name of the sphere.
+     * @param center
+     *            Center of the sphere.
+     * @param zSamples
+     *            The number of samples along the Z.
+     * @param radialSamples
+     *            The number of samples along the radial.
+     * @param radius
+     *            The radius of the sphere.
+     * @param useEvenSlices
+     *            Slice sphere evenly along the Z axis
+     */
+    public Sphere(String name, Vector3f center, int zSamples,
+            int radialSamples, float radius, boolean useEvenSlices) {
         super(name);
-        updateGeometry(center, zSamples, radialSamples, radius);
+        updateGeometry(center, zSamples, radialSamples, radius, useEvenSlices);
     }
 
     /**
@@ -152,7 +187,7 @@

     /**
      * @return Returns the textureMode.
      */
-    public int getTextureMode() {
+    public TextureMode getTextureMode() {
         return textureMode;
     }
 
@@ -168,7 +203,8 @@

         radius = capsule.readFloat("radius", 0);
         center = (Vector3f) capsule
                 .readSavable("center", Vector3f.ZERO.clone());
-        textureMode = capsule.readInt("textureMode", TEX_ORIGINAL);
+        useEvenSlices = capsule.readBoolean("useEvenSlices", false);
+        textureMode = capsule.readEnum("textureMode", TextureMode.class, TextureMode.Original);
     }
 
     /**
@@ -182,7 +218,7 @@

      * @deprecated Use {@link #updateGeometry(Vector3f,int,int,float)} instead
      */
     public void setData(Vector3f center, int zSamples, int radialSamples, float radius) {
-        updateGeometry(center, zSamples, radialSamples, radius);
+        updateGeometry(center, zSamples, radialSamples, radius, false);
     }
 
     /**
@@ -220,7 +256,13 @@

         // generate the sphere itself
         int i = 0;
         for (int iZ = 1; iZ < (zSamples - 1); iZ++) {
-            float fZFraction = -1.0f + fZFactor * iZ; // in (-1,1)
+            float fAFraction = FastMath.HALF_PI * (-1.0f + fZFactor * iZ); // in (-pi/2, pi/2)
+            float fZFraction;
+            if (useEvenSlices)
+                fZFraction = -1.0f + fZFactor * iZ; // in (-1, 1)
+            else
+                fZFraction = FastMath.sin(fAFraction); // in (-1,1)
+
             float fZ = radius * fZFraction;
 
             // compute center of slice
@@ -252,14 +294,20 @@

                     getNormalBuffer().put(-kNormal.x).put(-kNormal.y).put(
                             -kNormal.z);
 
-                if (textureMode == TEX_ORIGINAL)
+                if (textureMode == TextureMode.Original)
                     getTextureCoords().get(0).coords.put(fRadialFraction).put(
                             0.5f * (fZFraction + 1.0f));
-                else if (textureMode == TEX_PROJECTED)
+                else if (textureMode == TextureMode.Projected)
                     getTextureCoords().get(0).coords.put(fRadialFraction).put(
                             FastMath.INV_PI
                                     * (FastMath.HALF_PI + FastMath
                                             .asin(fZFraction)));
+                else if (textureMode == TextureMode.Polar) {
+                    float r = (FastMath.HALF_PI - FastMath.abs(fAFraction)) / FastMath.PI;
+                    float u = r * afCos[iR] + 0.5f;
+                    float v = r * afSin[iR] + 0.5f;
+                    getTextureCoords().get(0).coords.put(u).put(v);
+                }
 
                 i++;
             }
@@ -267,15 +315,19 @@

             BufferUtils.copyInternalVector3(getVertexBuffer(), iSave, i);
             BufferUtils.copyInternalVector3(getNormalBuffer(), iSave, i);
 
-            if (textureMode == TEX_ORIGINAL)
+            if (textureMode == TextureMode.Original)
                 getTextureCoords().get(0).coords.put(1.0f).put(
                         0.5f * (fZFraction + 1.0f));
-            else if (textureMode == TEX_PROJECTED)
+            else if (textureMode == TextureMode.Projected)
                 getTextureCoords().get(0).coords.put(1.0f)
                         .put(
                                 FastMath.INV_PI
                                         * (FastMath.HALF_PI + FastMath
                                                 .asin(fZFraction)));
+            else if (textureMode == TextureMode.Polar) {
+                float r = (FastMath.HALF_PI - FastMath.abs(fAFraction)) / FastMath.PI;
+                getTextureCoords().get(0).coords.put(r+0.5f).put(0.5f);
+            }
 
             i++;
         }
@@ -293,8 +345,14 @@

             getNormalBuffer().put(0).put(0).put(1);
 
         getTextureCoords().get(0).coords.position(i * 2);
-        getTextureCoords().get(0).coords.put(0.5f).put(0.0f);
 
+        if (textureMode == TextureMode.Polar) {
+            getTextureCoords().get(0).coords.put(0.5f).put(0.5f);
+        }
+        else {
+            getTextureCoords().get(0).coords.put(0.5f).put(0.0f);
+        }
+
         i++;
 
         // north pole
@@ -305,7 +363,12 @@

         else
             getNormalBuffer().put(0).put(0).put(-1);
 
-        getTextureCoords().get(0).coords.put(0.5f).put(1.0f);
+        if (textureMode == TextureMode.Polar) {
+            getTextureCoords().get(0).coords.put(0.5f).put(0.5f);
+        }
+        else {
+            getTextureCoords().get(0).coords.put(0.5f).put(1.0f);
+        }
     }
 
     /**
@@ -375,8 +438,22 @@

     /**
      * @param textureMode
      *            The textureMode to set.
+     * @Deprecated Use enum version of setTextureMode
      */
+    @Deprecated
     public void setTextureMode(int textureMode) {
+        if (textureMode == TEX_ORIGINAL)
+            this.textureMode = TextureMode.Original;
+        else if (textureMode == TEX_PROJECTED)
+            this.textureMode = TextureMode.Projected;
+        setGeometryData();
+    }
+
+    /**
+     * @param textureMode
+     *            The textureMode to set.
+     */
+    public void setTextureMode(TextureMode textureMode) {
         this.textureMode = textureMode;
         setGeometryData();
     }
@@ -390,10 +467,15 @@

      * @param radius the radius of the sphere.
      */
     public void updateGeometry(Vector3f center, int zSamples, int radialSamples, float radius) {
+        updateGeometry(center, zSamples, radialSamples, radius, false);
+    }
+
+    public void updateGeometry(Vector3f center, int zSamples, int radialSamples, float radius, boolean useEvenSlices) {
         this.center = center != null ? center : new Vector3f();
         this.zSamples = zSamples;
         this.radialSamples = radialSamples;
         this.radius = radius;
+        this.useEvenSlices = useEvenSlices;
         setGeometryData();
         setIndexData();
     }
@@ -405,7 +487,8 @@

         capsule.write(radialSamples, "radialSamples", 0);
         capsule.write(radius, "radius", 0);
         capsule.write(center, "center", Vector3f.ZERO);
-        capsule.write(textureMode, "textureMode", TEX_ORIGINAL);
+        capsule.write(useEvenSlices, "useEvenSlices", false);
+        capsule.write(textureMode, "textureMode", TextureMode.Original);
     }
 

7

Great contribution, thanks!



Definitely switch to using an enumeration. This is more consistent with the API now, this must have been missed when doing the other enums. If there are any specific public methods that use the constant, then just deprecate them and rewrite using the enum.

8

Okay, no problem.  I'll add the alternate constructor to support the old behavior (just in case.)  I'll also switch that to an enum to be more JME2 compliant.  Only one public method should be impacted: setTextureMode()



One more question though.  Should I leave the existing setTextureMode(int) and static final vars TEX_ORIGINAL and TEX_PROJECTED and just mark it @Deprecated, or should they be removed and just break compilation if anyone's using them?  Either way I'll create a new setTextureMode(enum) and the new polar mode will only be supported with the new call.



Will send the new diffs shortly.

9

Deprecate setTextureMode(int) and just fill it with an if/else that forwards to setTextureMode(enum) and then deprecate TEX_ORIGINAL and TEX_PROJECTED. We can remove them later, but we should give people the chance to transition at their own pace.


10

Will do.  I also noticed a couple other things that'll break:



getTextureMode() – I have to remove the version that returns an int (can't just deprecate) but I suspect few people actually use this.



Object persistence (read / write) – this will break because the textureMode int turns into an enum.  I don't know if this is a big deal.

11

Well I have to admit I was secretly hoping someone would chime in and say "yeah, nevermind about that useEvenSlices stuff".  :slight_smile:



But, if there are no other objections I guess I'll commit what I've got here.  Thanks all!

12

heh sorry it was just an idea, i guess we can all also live without it, but it dosent hurt to have the option  :slight_smile: