#RoastMyCode revolution of function 3D visualization

0xDEADBEEF

Hi everyone, I found an old project of mine, that I'd like to get some criticism on. I can clearly see that the code is bad, but would like to know/see what you would have done instead of what I did. So please #RoastMyCode:

Android_Plot.pde http://pastebin.com/8NmxjRQn

Plot3D.pde http://pastebin.com/pWEuPk78

kfrajer

It is an interesting concept. I like to say your code is not constrained to android mode but could be run in Processing as well.

I think a better exercise is if you comment what you would do different and people could comment on those thoughts.

I added the code below so people can run it themselves.

Kf

//////////////////////////////////////////////
//              Global variables
//////////////////////////////////////////////

// 3D plot object
plot3D plot;


//////////////////////////////////////////////
//                  Setup
//////////////////////////////////////////////

void setup() {
  size(displayWidth, displayHeight, P3D);
  frameRate(240);

  // 3D plot object
  int xCenter = width / 2;
  int yCenter = height / 2;
  int vertices = 50;
  int rings = 1;
  int xBegin = -180;
  int xEnd = 180;
  int drawMethod = LINES;
  float xRatio = 20;
  float yRatio = 5;
  plot = new plot3D(xCenter, yCenter, vertices, rings, xBegin, xEnd, drawMethod, xRatio, yRatio);
}

//////////////////////////////////////////////
//                  Draw
//////////////////////////////////////////////

void draw() {

  // Set bagground to blank color
  background(255);

  // Display and update the plot object
  plot.run();
}



/**
  Class creates a 3D plot/figure of a functions revolution
 around the x-axis. Construct the object and call run() to
 display the 3D plot.

 The following @ params must be set when calling the constructor:
 int xCenter     // (x;y) center of the plot
 int yCenter     // (x;y) center of the plot
 int vertices    // The plots vertices - High value may decrease performance
 int rings;      // The plots x-axis quality - Low value may decrease performance
 int xBegin;     // Starting point from where the math function should be rotated
 int xEnd;       // Ending point from where the math function should be rotated
 int drawMethod; // Either POINTS, LINES, TRIANGLES, TRIANGLE_FAN, TRIANGLE_STRIP, QUADS, or QUAD_STRIP
 float xRatio;   // Increase/Decrease the plots relative x-axis size
 float yRatio;   // Increase/Decrease the plots relative y-axis size

 */

class plot3D {

  /**
  --- Variables ---
   */

  // Used for the rotation of the plot
  float rX = 0;
  float rY = 0;
  float tempX = 0;
  float tempY = 0;
  int xStartClick = 0;
  int yStartClick = 0;
  boolean rotateInProgress = false;

  // Used as plot settings
  int xCenter;
  int yCenter;
  int vertices;
  int rings;
  int xBegin;
  int xEnd;
  int drawMethod;
  float xRatio;
  float yRatio;

  // PShape object that will be become the 3D figure
  private PShape plot;

  // Used by function settingsChanged()
  private int old_vertices;
  private int old_xBegin;
  private int old_xEnd;
  private int old_rings;
  private float old_xRatio;
  private float old_yRatio;

  /**
  --- Constructor ---
   */

  plot3D(int _xCenter, int _yCenter, int _vertices, int _rings, int _xBegin, int _xEnd, int _drawMethod, float _xRatio, float _yRatio) {
    xCenter = _xCenter;
    yCenter = _yCenter;
    vertices = _vertices;
    rings = _rings;
    xBegin = _xBegin;
    xEnd = _xEnd;
    drawMethod = _drawMethod;
    xRatio = _xRatio;
    yRatio = _yRatio;
  }

  /**
  --- Methods ---
   */

  void run() {
    // Update the PShape plot object if settings has changed.
    if (settingsChanged()) {
      updatePShape();
    }

    pushMatrix();

    // Set objects relative position to the center of the screen
    translate(xCenter, yCenter);

    // Set rotation
    if (mousePressed)
    {
      rotatePlot();
    } else {
      rotateInProgress = false;
      rX += tempX;
      rY += tempY;
      tempX = 0;
      tempY = 0;
    }
    rotateX(rX + tempX);
    rotateY(rY + tempY);

    // Draw the 3dplot
    shape(plot);

    popMatrix();
  }

  private void updatePShape() {
    plot = createShape();
    plot.beginShape(drawMethod);

    // Calculate new coordinates for the 3Dplot
    float angle = 360 / vertices;
    for (int x = xBegin; x < xEnd; x += rings) {
      for (int j = 0; j < vertices + 1; j++) {
        // Calculate y and z coordinates
        float y = sin( radians(angle * j) ) * fx(x / xRatio) * yRatio;
        float z = cos( radians(angle * j) ) * fx(x / xRatio) * yRatio;
        plot.vertex(x, y, z);
      }
    }

    plot.endShape();
  }

  void rotatePlot() {
    // Get rotation start coordinate if rotation
    // is not allready in progress
    if (!rotateInProgress) {
      xStartClick = mouseX;
      yStartClick = mouseY;
      rotateInProgress = true;
    }

    // Set temp rotation to the amount the mouse has
    // travelled since mousePressed == true started
    tempY = 0.004 * (mouseX - xStartClick);
    tempX = -0.004 * (mouseY - yStartClick);
  }

  boolean settingsChanged() {
    if (old_vertices != vertices ||
      old_xBegin != xBegin ||
      old_xEnd != xEnd ||
      old_rings != rings ||
      old_xRatio != xRatio ||
      old_yRatio != yRatio) {

      old_vertices = vertices;
      old_xBegin = xBegin;
      old_xEnd = xEnd;
      old_rings = rings;
      old_xRatio = xRatio;
      old_yRatio = yRatio;

      return true;
    } else {
      return false;
    }
  }

  /** 
   This function, fx(), should be replaced by
   an expression parser lib. so that
   the user can rotate any arbitrary
   math function
   */
  float fx(float x) {
    return sin(x)*20*x*x;
  }
}

Lord_of_the_Galaxy

Great improvement @kfrajer. It's easier to use code this way. (It goes without saying that the original code was also good, but the proper formatting and commenting added a new level of detail)

kfrajer

@Lord_of_the_Galaxy I didn't change the code, not a bit. I just copied it and pasted it here from the provided link so people can run it themselves (try to make it easier for other people to see it themselves). I didn't add any comments and I am more interested to learn what the OP has to say about its design.

Kf

Lord_of_the_Galaxy

@kfrajer The formatting has changed. That's what I meant. I guess that was automatic.

0xDEADBEEF

1. Lines like this should be shortened: plot3D(int _xCenter, int _yCenter, int _vertices, int _rings, int _xBegin, int _xEnd, int _drawMethod, float _xRatio, float _yRatio) { I wish you could parse a struct or something to the constructor. I'm not sure what the approach should be in Processing.

2. I think that the plot3D should have different methods. Some for setting the class' variables etc. settingsChanged() and rotatePlot() should be private methods.

4. I'd like rotatePlot to not depend on variables 'public' inside the class plot3d if it's possible.

5. Comments should be homogeneous.

Chrisir

use PVector more often to bundle all the x,y lines that belong together

Chrisir

plot3D should / could take a class as parameter

0xDEADBEEF

@Chrisir why put x,y and (,z) in a PVECTOR? The lines are calculated once when settingsChanged() and are only used by createShape(); stuff once. I don't see why putting the lines into a PVECTOR arraylist and then looping trough that arraylist for createShape makes sense?

Chrisir

I mean all of the code.

You can almost cut it by half in using PVectors.

Eg lines 18 to 26 in the 2nd post from the top and then nearly all variables following

Conceptually I also think PVector is clearer since you have in one class what belongs together

0xDEADBEEF

Hmm... Could you please elaborate, @Chrisir? I don't see how rotation vars etc. could be replaced by using the PCVECTOR class. PVECTOR::rotate exists, but only rotates 2D. Are you sure this is the appropriate thing to do thinking performance?

Chrisir

I am just talking about the storage of variables- just instead of centerx,centery use PVector center

center.x and center.y

0xDEADBEEF

@chrisir arh, I see. That would definitely be 'nicer'. But isn't it a bit of a waste to create and instances of the relatively big class just to store to tiny vars?

Chrisir

I don't know

GoToLoop

... relatively big class...

When we use new (or some other hack) to instantiate any class or array, the actual contents of the object created are just made outta non-static fields, plus a header. @-)

All of the rest of the class' members stay in the class. I-)

All methods, constructors & static fields aren't instantiated at all! \m/

GoToLoop

However, Processing's PVector got an extra useless field called array[]: :-&
transient protected float[] array;

Even though it's forever null all the time, until we actually invoke method array(), it still wastes 4 bytes of RAM. X(

Therefore, if we don't need PVector's methods, we're better of creating another class to have x, y & z fields only, w/o any useless crap fields! =P~

kfrajer

Cross-linking it to your previous comment @GoToLoop: https://forum.processing.org/two/discussion/comment/92271/#Comment_92271

Kf

0xDEADBEEF

@GoToLoop Some people don't respect the resources they've been given by Intel/AMD. I will of course create a class that can do just that!

Chrisir

PVector wouldn't take much space / processor time

as gotoloop wrote:

All methods, constructors & static fields aren't instantiated at all!

(by the way, "wastes 4 bytes of RAM" - four bytes is virtually nothing)

I bet, you can't measure a difference between PVector and your own class PVectorSimple

;-)

koogs

(by the way, "wastes 4 bytes of RAM" - four bytes is virtually nothing)

but there are, roughly, 11 places that could be replaced by pvectors. so that's 44 bytes wasted 8)

what's with those occasional radial lines? they look like they shouldn't be there.

GoToLoop

• The hearsay is that all Java objects got a header of 8 or 12 bytes. Gonna believe 12 here... 3:-O
• And due to 8-byte alignment, even an empty object w/ just 12-byte header, it ends up using 16 bytes anyways.
• Class PVector got 4 non-static fields. 3 float & 1 array reference.
• Each 1 of those 4 grabs 4 bytes: 4 * 4 = 16 bytes.
• Plus 12 bytes from header we've got: 12 + 16 = 28.
• However, the 8-byte alignment padding claims 4 more bytes.
• Thus each PVector object probably occupies 32 bytes total.
• However, if that extra 100% useless 4th field would get removed, instead of 32 bytes, it'd be 8 bytes less: 24 bytes each 1! @-)
• That is: 3 fields * 4 bytes + 12 bytes header = 12 + 12 = 24.
• Since 24 is already aligned to 8 bytes, final total bytes = 24! \m/

GoToLoop

According to https://www.SlideShare.net/cnbailey/memory-efficient-java
An object's header is indeed 12 bytes for 32-bit Java:
And it doubles for 64-bit. That is, 24 bytes total for its header:
Since I'm using the 64bit PDE's version, each PVector is now: 24 + 4*4 = 24 + 16 = 40 bytes.
No need for 8-bit alignment padding this time like the 32bit version. =P~

But still, for 32bit Java PDEs, removing the 4th useless array[] field would net 8 bytes off, from 32 bytes to 24 bytes for each PVector object. [-(

Chrisir

But still, for 32bit Java PDEs, removing the 4th useless array[] field would net 8 bytes off, from 32 bytes to 24 bytes for each PVector object.

So each appearance 8 bytes, so approx. 11*8 bytes = 88 bytes saved.

I still don't think that's a lot. But thank you anyway.

koogs

it's 24 bytes, total, for a PVector according to this:

http://www.javaworld.com/article/2077408/core-java/sizeof-for-java.html

8 overhead, 4 for each float, 4 for the array ref.

GoToLoop

• Indeed, those slide images seem like are for IBM's Java or something.
• What we're interested in is the Oracle's Java. Maybe OpenJDK as well. ;)
• I've found another object memory scheme at this Gist link:
  https://Gist.GitHub.com/arturmkrtchyan/43d6135e8a15798cc46c
• If those are indeed correct for Oracle's Java, we've got these 2 situations now:

a) Class PVector for 32-bit Java PDE: (Header = 8 bytes (64 bits))

8 + 4*4 = 8 + 16 = 24 bytes total. No padding needed!

If we theorize it w/o the 4th extra field we'd get:
8 + 3*4 = 8 + 12 = 20 bytes + 4 padding = 24 bytes total. No difference due to 8-byte align padding!

b) Class PVector for 64-bit (compressed) Java PDE: (Header = 12 bytes (96 bits))

12 + 4*4 = 12 + 16 = 28 bytes + 4 padding = 32 bytes total.

Theorizing again w/o the 4th extra array[] field we get now:
12 + 3*4 = 12 + 12 = 24 total. No padding needed!
Difference is 32 - 24 = 8 bytes off!

So for 64-bit Java w/ compressed addresses turned on (default), we're wasting 8 bytes for each PVector object created due to its extra useless 4th field! X(

jeremydouglass

Whether or not to use PVector -- and both approaches are valid -- is probably a readability / maintainability issue, not a performance issue.

A sketch with a PVector "wastes" 8 bytes. A sketch with 10,000 PVectors uses an extra 80 kilobytes. This is still nothing -- many orders of magnitude away from ever becoming a memory optimization concern even on a mobile system. Saying otherwise seems like a classic example of premature optimization.

Show me a sketch with millions of PVectors and a performance bottleneck, and then let's talk about replacing a class to save 8 bytes.

Lord_of_the_Galaxy

Good point. Nonetheless, for obvious reasons, it is not a good programming practice to have such wasteful variables in a program. For example, you could just add a few extra but mostly useless variables to your own custom class, but it won't be sensible to do so.

GoToLoop

A sketch with a PVector "wastes" 8 bytes.

I was specifically ranting about the 4th array[] 100% unused field inside PVector class.

But if we compare 3 float x, y & z "unbound" fields against 1 PVector "encapsulated" x, y & z fields, plus its empty array[] field, the diff. isn't 8 bytes only! [-X

Each float is 4 bytes. So 3 float fields = 12 bytes only.
But if we replace those 3 float fields w/ 1 PVector object, as I had already calculated in my previous answer, it's 32 bytes each for the 64bit Java PDE! :-&

The encapsulation cost is then: 32 bytes - 12 bytes = 20 wasted bytes! :-t

And even worse, if we also count the field variable which stores the PVector object, we got an extra cost of 4 bytes! @-)

So it's not 32 bytes, but actually 36 bytes! b-(
And wasted bytes are now 24, for each time we choose 1 PVector over 3 float fields!!! :-h

Lord_of_the_Galaxy

In simpler words, sometimes you have to make your code less efficient just to make it easier to understand.

0xDEADBEEF

So doing that saved in total 14 lines of code. Using the PVector class brings other problems, as it uses the float data type: for (int x = xInterval.x; x < xInterval.y; x += rings) { Needs to be casted or something: for (int x = (int)xInterval.x; x < xInterval.y; x += rings) {

Do you guys really think the code is prettier now?

//////////////////////////////////////////////
//                  Imports
//////////////////////////////////////////////


//////////////////////////////////////////////
//              Global variables
//////////////////////////////////////////////

// 3D plot object
plot3D plot;

// xRatio and yRatio buttons
Button B_xRatioPlus, B_xRatioMinus, B_yRatioPlus, B_yRatioMinus;

//////////////////////////////////////////////
//                  Setup
//////////////////////////////////////////////

void setup() {
  size(displayWidth, displayHeight, P3D);
  frameRate(240);

  // 3D plot object
  PVector center = new PVector(width / 2, height / 2);
  PVector xInterval = new PVector(-180, 180);
  PVector ratio = new PVector(20,5);
  int vertices = 50;
  int rings = 1;
  int drawMethod = LINES;
  plot = new plot3D(center, xInterval, ratio, vertices, rings, drawMethod);
}

//////////////////////////////////////////////
//                  Draw
//////////////////////////////////////////////

void draw() {

  // Set bagground to blank color
  background(255);

  // Display and update the plot object
  plot.run();
}

/**
  Class creates a 3D plot/figure of a functions revolution
  around the x-axis. Construct the object and call run() to
  display the 3D plot.

  The following @ params must be set when calling the constructor:
  PVector Center;      // (x;y) center of the plot
  PVector xInterval;   // x-axis interval for the math function that is rotated
  PVector ratio;       // Increase/Decrease the plots relative x- and y-axis size
  int vertices;        // The plots vertices - High value may decrease performance
  int rings;           // The plots x-axis quality - Low value may decrease performance
  int drawMethod;      // Either POINTS, LINES, TRIANGLES, TRIANGLE_FAN, TRIANGLE_STRIP, QUADS, or QUAD_STRIP

 */

class plot3D {

  /**
  --- Variables ---
   */

  // Used as plot settings
  PVector center;
  PVector xInterval;
  PVector ratio;
  int vertices;
  int rings;
  int drawMethod;

  // Used for the rotation of the plot
  PVector rotation = new PVector(0,0);
  PVector tempRotation = new PVector(0,0);
  PVector startClick = new PVector(0,0);
  boolean rotateInProgress = false;

  // PShape object that will be become the 3D figure
  private PShape plot;

  // Used by function settingsChanged()
  private PVector old_xInterval;
  private PVector old_ratio;
  private int old_vertices;
  private int old_rings;

  /**
  --- Constructor ---
   */

  plot3D(PVector _center, PVector _xInterval, PVector _ratio, int _vertices, int _rings, int _drawMethod) {
    center = _center;
    xInterval = _xInterval;
    ratio = _ratio;
    vertices = _vertices;
    rings = _rings;
    drawMethod = _drawMethod;
  }

  /**
  --- Methods ---
   */

  void run() {
    // Update the PShape plot object if settings has changed.
    if (settingsChanged()) {
      updatePShape();
    }

    pushMatrix();

    // Set objects relative position to the center of the screen
    translate(center.x, center.y);

    // Set rotation
    if (mousePressed)
    {
      rotatePlot();
    } else {
      rotateInProgress = false;
      rotation.x += tempRotation.x;
      rotation.y += tempRotation.y;
      tempRotation.x = 0;
      tempRotation.y = 0;
    }
    rotateX(rotation.x + tempRotation.x);
    rotateY(rotation.y + tempRotation.y);

    // Draw the 3dplot
    shape(plot);

    popMatrix();
  }

  private void updatePShape() {
    plot = createShape();
    plot.beginShape(drawMethod);

    // Calculate new coordinates for the 3Dplot
    float angle = 360 / vertices;
    for (int x = (int)xInterval.x; x < xInterval.y; x += rings) {
      for (int j = 0; j < vertices + 1; j++) {
        // Calculate y and z coordinates
        float y = sin( radians(angle * j) ) * fx(x / ratio.x) * ratio.y;
        float z = cos( radians(angle * j) ) * fx(x / ratio.x) * ratio.y;
        plot.vertex(x, y, z);
      }
    }

    plot.endShape();
  }

  void rotatePlot() {
    // Get rotation start coordinate if rotation
    // is not allready in progress
    if (!rotateInProgress) {
      startClick.x = mouseX;
      startClick.y = mouseY;
      rotateInProgress = true;
    }

    // Set temp rotation to the amount the mouse has
    // travelled since mousePressed == true started
    tempRotation.y = 0.004 * (mouseX - startClick.x);
    tempRotation.x = -0.004 * (mouseY - startClick.y);
  }

  boolean settingsChanged() {
    if (old_vertices != vertices ||
      old_xInterval.x != xInterval.x ||
      old_xInterval.y != xInterval.y ||
      old_rings != rings ||
      old_ratio.x != ratio.x ||
      old_ratio.y != ratio.y) {

      old_vertices = vertices;
      old_xInterval = xInterval;
      old_rings = rings;
      old_ratio = ratio;

      return true;
    } else {
      return false;
    }
  }

  /**
     This function, fx(), should be replaced by
     an expression parser lib. so that
     the user can rotate any arbitrarotation.y 
     math function
   */
  float fx(float x) {
    return sin(x)*20*x*x;
  } 

}

Lord_of_the_Galaxy

P.S. To prevent "@params" from becoming a hyperlink, put a space between @ and params. Like this - @ params.

The code is quite pretty looking right now. :)>-

0xDEADBEEF

Can someone explain why the function settingsChange isn't true if I change the PVector types? E.g. does something like this in draw(): plot.ratio.x += 2; It changes both old_ratio.x and ratio.x ???

settingschanged() works with other variable types like int ring;

GoToLoop

@ line #183 old_ratio = ratio;, you're making old_ratio to point to the same PVector object reference which ratio already points to! :-@

If your intention is for each 1 to point to its own PVector object, you're gonna need to clone ratio's w/ get() or copy(), and assign that clone to old_ratio: *-:) old_ratio = ratio.get();

koogs

You can't use == on objects. Use .equals() instead.

It changes both old_ratio.x and ratio.x ???

Probably also related to the fact that pvector is an object. Sounds like you are assigning one pvector to another, so they now both refer to the same object.

0xDEADBEEF

@GoToLoop Arh, okay. So to clarify: I'm wrongfully deleting/freeing the old memory that the PVector held and setting it to point towards the same location in the RAM as the new one?

@koogs I was checking each of the PVectors fields, but equals() is nicer, thanks.

GoToLoop

@0xDEADBEEF, let's analyse the lifespan of 1 PVector object within your sketch, shall we? O:-)

• At line #27 you've got PVector ratio = new PVector(20, 5);
• So a new PVector is instantiated, and thus a new derived object is created outta class PVector.
• For illustration purposes, let's just say that newborn object was created starting at memory address 150 000.
• Hence setup()'s local variable ratio is assigned the value 150 000, pointing to the new PVector object.
• At line #31, plot = new plot3D(center, xInterval, ratio, vertices, rings, drawMethod);, a plot3D object is created at some other memory block location.
• To its constructor, you pass setup()'s local variable ratio as its 3rd argument.
• Given ratio is currently holding the value 150 000, that's exactly the value which is received by that constructor.
• Within that constructor, at line #98 ratio = _ratio;, we can see constructor's parameter _ratio is assigned to plot3D's field ratio.
• Given constructor's parameter _ratio received value 150 000 from setup()'s local variable ratio, and it is consequently assigned to plot3D's field ratio, the latter is now pointing to that very same PVector object created within setup() at memory address 150 000. #:-S
• Let's scroll down now to line #183 old_ratio = ratio;.
• Since ratio probably still holds the value 150 000 it had received within constructor block, now field old_ratio is assigned that same value.
• Thus both ratio & old_ratio have the same value and they're pointing to the very same PVector object on that memory location.
• Therefore ratio & old_ratio are alias to the same object! :-\"
• If we mutate any PVector's fields using 1 of those 2 alias, we'll realize that the other alias "sees" those same modifications! @-)

Chrisir

This issue can be tackled with copy()?

PVector e, r=new PVector(2, 2); 

e=r.copy ();

println(e.x); 
r.set(7, 2); 
println(e.x); 

also those lines can be simplified

  rotation.x += tempRotation.x;
  rotation.y += tempRotation.y;
  tempRotation.x = 0;
  tempRotation.y = 0;

like

    rotation.add(tempRotation); 
    tempRotation.set(0,0); 

?

not tested