#RoastMyCode revolution of function 3D visualization

edited March 25 in Android Mode

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



  • edited March 22

    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.


    //              Global variables
    // 3D plot object
    plot3D plot;
    //                  Setup
    void setup() {
      size(displayWidth, displayHeight, P3D);
      // 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
      // Display and update the plot object
      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
     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()) {
        // Set objects relative position to the center of the screen
        translate(xCenter, yCenter);
        // Set rotation
        if (mousePressed)
        } else {
          rotateInProgress = false;
          rX += tempX;
          rY += tempY;
          tempX = 0;
          tempY = 0;
        rotateX(rX + tempX);
        rotateY(rY + tempY);
        // Draw the 3dplot
      private void updatePShape() {
        plot = createShape();
        // 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);
      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;
  • edited March 22

    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)

  • @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.


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

  • edited March 23

    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.

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

  • plot3D should / could take a class as parameter

  • @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?

  • 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

  • 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?

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

    center.x and center.y

  • @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?

  • I don't know

  • ... 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/

  • edited March 25

    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~

  • @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!

  • edited March 25

    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


  • (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.

  • edited March 25
    • 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/
  • edited March 25

    According to https://www.SlideShare.net/cnbailey/memory-efficient-java
    An object's header is indeed 12 bytes for 32-bit Java:
    javaone-2013-memory-efficient-java-7-638 And it doubles for 64-bit. That is, 24 bytes total for its header:
    javaone-2013-memory-efficient-java-10-638 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. [-(

  • 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.

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


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

  • edited March 25
    • 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:
    • 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(

  • 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.

  • 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.

  • edited March 27

    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

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

Sign In or Register to comment.