Prepared by the markers: Ben Porter and Jon McCormack.
Specific comments and feedback are provided when you get your mark. Here are some general comments and further explanation of common mistakes for the Superformula Assignment.
Generally everyone went as well as expected, however the quality of the software design and coding was, in general, poor. Also some OpenGL concepts seemed to be misunderstood. The following is a summary of the major issues people had, and suggestions for fixing them.
Some submissions showed a reasonable understanding of how to use make and
Makefiles. Others looked like they were copied from the first google hit under "poor
makefile". Hence, a few tips are in order. Here's what a simple Makefile for
the assignment should look like:
clean: -rm -f $(PROGRAM) $(OBJS)
Note that the clean target doesn't do anything silly like rm *.o.
This makefile doesn't
look at header file dependencies. If it did it would be better. Likewise there is a better way to convert SRCS to OBJS. I'll leave
these modifications as an exercise.
README files should include the following information:
The README does not need to be longer than an A4 page. It should be submitted in plain text, suitable for viewing in a 80 column text editor (e.g. vim).
Also a general point: please read the assignment sheet carefully. Please listen to the advice given by the lecturer in the lectures and tutes. If you are unsure please ask for advice. A number of submissions did not read the specifications correctly or interpreted them incorrectly.
Most submissions displayed a reasonable basic understanding of OpenGL in 2D. A number of submissions had trouble with the resize and zoom using multiple viewports. The key thing to understand is how a primitive goes from world co-ordinates to screen co-ordinates and then pixels set on the screen. The basic process should be:
glViewportGL_PROJECTIONgluOrtho2D to map world coordinates to screen coordinates within the
viewport. This involves matching the aspect ratio of the viewport (calculated
from the screen resolution and coverage of the current viewport. If the view
can zoom this will also needed to be included in the calculation.GL_MODELVIEW
There were a couple of problems here. The first was that some people were recompiling
their display lists every time display() was called – this usually implies
that your geometry/drawing instructions are too dynamic to utilise the benefits
of display lists, or that you are incorrectly using them. Display lists should
be used for geometry that doesn't change every frame. The other problem is
less critical but still a valid point – almost everyone was calling glNewList with
identifiers that they created. This is prone to error and glGenLists should
be used for each new single or set of identifiers that you need.
Consider the following code:
glTranslatef(1,0,0);
drawSuperFormula(...);
glTranslatef(-1,0,0);
drawSphere(...);
...we can all see clearly what it does (draws a superformula at (1,0,0) and draws a box at (0,0,0)). However, we are all assuming that drawSuperFormula(...) doesn't modify the ModelView or Projection matrices upon returning --- this is a logical (but unspoken) assumption about the behaviour of drawSuperFormula(). In fact, we can assume the same about any function called drawXXX() simply because of the name of the function. Unfortunately many submissions violated this by immediately calling glLoadIdentity() upon entering a drawXXX() function, thus destroying the current transformation matrix. In conclusion, if you modify something in a function, make sure you restore it to its original state UNLESS the function name/description implies that it is going to modify that state (e.g., a function called setGLColour(...) is obviously going to modify the current colour). You can use glPush/PopAttribs to store/restore all other state information (including colour, lighting, texture info, ...).
On the other side of the function call, we can also assume that the caller has set up the transformation matrices appropriately to position/scale/rotate the object in the correct manner ... so all you need to do is draw the object at the local origin. This is how glutCube, glutSphere, etc, all work.
void drawSuperFormula(...)
{
// we assume the user has selected the modelview matrix as this is a "drawXXX" call..
// store current state
glPushMatrix();
... draw the super formula here ...
// restore transformation matrix
glPopMatrix();
}
Writing robust and efficient programs still seems to be a challenge for many of you. Here are some common mistakes.
Please refer to your software engineering textbooks for good style and usage
of global variables. The general idea is that you shouldn't be sharing global
variables between files, you should have a naming convention which identifies
them as global (e.g., append a g_ to each), you shouldn't have more than say
7 (and if you have more than this then you should make your code more modular
(with e.g., a global variable ViewportParameters that holds all the globals
related to viewport parameters)). As someone reading your code, I should be
able to look at a single function and figure out what it does by examining
its input's (parameters), its internal logic, and its output – I can't do this
if some of its inputs/outputs are via globally shared variables, and the entire
logic of your system becomes a tangled mess. As a general rule, if your system
has lots of globals then it is poorly designed.
This is also related to the above point about global variables. You should design your system out of modular components -- these components should be clearly defined by an interface (a set of functions) and should be general enough to be able to be used in other places. Separate modules should go in separate files, and compiled into separate objects. A good example of a module which I saw in a few submissions is a Viewport module. This is either a viewport class, or a set of viewport related functions and data structures, that took over all the responsibility of setting up and managing the different viewports in the system. You could have a further module Window that is responsible for managing a collection of viewports and assigning them different rendering tasks. It was disappointing and confusing that many submissions were in a single large source file.
A further point I want to make is on loosely coupled systems. Your modules should each do a large amount of (specific) work, and you should limit the communication between different modules. One thing I saw a lot of is a huge list of get/set methods in the SuperFormula module, one for each parameter. Consider whether this was necessary in your design. For example, if you needed that data in order to print out the current parameters then maybe a function SuperFormula::toString() would be better ... thus loosening the coupling in the system and reducing the dependency on X to know about the different parameters that SuperFormula has.
The set of modules that I would have liked to see are:
Glut is not an object-oriented toolkit, which makes it a bit tricky to incorporate into an OO design. One particular problem is the use of function pointers in the callbacks ... we would ideally like to pass pointers to member functions of classes, but we can't. One solution to this problem is as follows...
class Window
{
Window* win;
public:
Window(int* argc, char** argv){win = this; glutInit(argc,argv); init();}
void init(){ ... blah blah ... glutDisplayCallback(&Window::DISPLAY);}
void run(){... glutMainLoop();}
static void DISPLAY(){win->display();}
};
...
void main(int argc, char** argv)
{
Window win(&argc,argv);
win.run();
}
In that code I am using static class functions as wrappers that call a singleton's appropriate member function. Now we can also derive a new class from Window and this solution will still work (if display() is virtual).
PI and other maths
Many people were defining their own pi constant. Both the C standard library math.h and the C++ library cmath supply
pi as M_PI.
If you want to square a number don't do
this: y = pow(x,2.0) its very inefficient and prone to rounding
errors. Use y = x * x; or better define a macro so you can say y
= SQUARED(x); or y = SQUARED(x + 2); [equivalent to
(x+2)(x+2)]
Some people were using their own custom linked list. The STL (for the C++ people) provides a powerful set of containers (e.g., a linked list std::list) that you should use – they make your code more readable, faster, and it's good practice!
GL_POLYGON was used by a large number of submissions. GL_POLYGON only works for convex polygons (a shape is convex if you can connect any two points inside the shape by a single straight line without crossing the shape boundary). The best solution for this assignment was to use GL_TRIANGLE_FAN.
Avoid the use of "magic numbers" in your code:
Consider statements like: glScale(0.61, 0.61, 0.61) or char
s[20]
This is bad programming. Its not clear why you need to scale by that amount.
In the second example its not clear
why you need an
array
of
exactly 20 characters. What if you need more space? The identifier name ('s')
gives no
indication
of
what the purpose of this array is. Here's a better solution:
#define STRING_BUFFER_SIZE 20
// some time later...
char tmpString[STRING_BUFFER_SIZE];
ASSERT(strlen(newString) < STRING_BUFFER_SIZE)).
If sometime later in your programs development you decide you need more space
for buffers you only need to change the STRING_BUFFER_SIZE at
one location, even though you might be using many temporary buffers throughout
your program.
Note that this solution is not applicable if you're using C++. In fact in
C++ the need to #define constants is very rare – use the const qualifier
because this gives you stricter type checking and more
traceable errors. Also, C++ has a built-in variable length
string type so you don't normally need temporary buffers.
The basic thing to remember is that if you find yourself putting any numeric or other constants in your code, consider defining them as constants in an associated header file.
Factorise code
I saw lots of code like this:
glRasterPos2f(-9.0, -1.5);
writetext("[+/-]: inc/dec theta");
glRasterPos2f(-9.0, -2.5);
writetext("[ s / S ]: inc/dec scale");
glRasterPos2f(-9.0, -3.5);
writetext("[up/dn]: move in y");
glRasterPos2f(-1.5, -3.5);
writetext("[left/right]: move in x");
glRasterPos2f(-9.0, -4.7);
writetext("[Z]: zoom in");
etc...
Why not add an parameter to writetext
to include a specification of the position? You've instantly cut the amount
of code you have to write in half! Better still, write a function that takes
strings with new lines in them and then calls writetext so then
you only need
one function call for all the text, which you can predefine (or even put in a
display list).
if-then-else chains
Another common mistake is using long if-then-else chains when a case
statement would be simpler and clearer. It also handles exceptional cases better
via the default: case.
Here's another example I saw a lot:
#define BLACK 0
#define GREEN 1
#define RED 2
...
if (choice == 1)
colour = BLACK;
else if (choice == 2)
colour = GREEN;
else if (choice == 3)
colour = RED;
else if (choice == 4)
... etc. etc. 20-30 more lines of this
Why not just say:
colour = choice - 1;
You just saved yourself typing in 30+ lines of code!
Even better change the colour constants to match the choice variable.
In many situations you have to set a variable or variables based on the value of some other variable, often a choice set. The best way to do this is to develop some intuitive mapping between the variable and choice set so you can use numeric or other functions/expressions rather than long if-then-else statements. I've written quite a bit of code over the years and I can't think of a time when I've had to have more than 3 successive it-then-else chains at any one level. So if you find yourself doing this, think of my smiling face telling you to STOP IT RIGHT AWAY!
Cut & Paste
Turn off or do not use cut and paste in your text editor when programming.
Real programmers don't cut and paste, they think about how they can turn
repeated code into functions :-).
Dynamic Memory
Pointers and dynamic allocation at run time are difficult concepts to understand
and require a fair bit of practice and time. Clearly, many of you still need
to "do the time". For example, I see quite a bit of this:
char * text = (char *)malloc(sizeof (char) * 25);
This is a waste of time. Malloc is used to allocate memory when you
don't know how much you'll need at compile time. Clearly in this case
we do know (we'll need 25 chars worth thanks, but see comments on magic numbers
in code). So this can be replaced with:
char text[BUFFER_LENGTH];
Where we would define BUFFER_LENGTH to be 25.
For the assignment, you need a dynamic data structure because you don't know at compile time how many sets of superformula primitives are required by the user. Using linked lists, queues, or dynamic vectors (arrays) are all acceptable solutions. It seems a number of people learnt how to do a linked list in 1st year and are still using the same code without understanding it (shame!). Basic data structures like these are the bread and butter of programming – if you want to call yourself a professional computer scientist you should know how to implement these basic data structures from scratch!
Analysis of some submissions also revealed things like calling free on the same pointer twice
(that is illegal) or allocating new memory to a pointer before freeing what it previously pointed to.
Pointer problems and memory leak (failing to free memory allocated to a program when it is no longer needed) are the two most common sources of bugs in C programs. That's why java was invented :-(
Static Initialisation in C++
A few of you decided to use C++ to do the assignment. Good choice, however
there is a common problem with a number of submissions due to static initialisation.
In this case an object representing a primitive
was created during static initialisation (i.e. before main is called, for
example when an object is defined at global scope). While this is of course
legal in C++ it can cause problems if your object's constructor calls any
OpenGL (or creates other objects that do). Since you haven't had a chance
to enter main yet, you haven't been able to call glutInit() or glutCreateWindow().
It is necessary to call these functions before making any OpenGL calls. The
solution here is not to put OpenGL calls in any classes where you might want
to do static initialisation. Alternatively, you can defer initialisation
until after you've set up OpenGL correctly.
Also be aware that the C++ standard does not specify the order of static initialisation
so if you have interdependencies between two different objects you want to
initalise before main is called (i.e. object A needs initialisation before
object B) you will be in trouble. What's
worse
is that such a scheme may work on some platforms and not others so it will
not always appear as a bug.
One more general comment about using C++ with GLUT. GLUT's use of callback functions does not work well with an OO approach, because you want to call objects (or their member functions) if an event happens (like resize, key press or display). As you can't send the address of a member function to a glut callback, it makes things a bit messy. Unfortunately, this lead a number of people to develop hybrid OO/procedural solutions which were ugly. If you are interested in how to do it better please come and see me during consultation.
The easier it is for someone to understand the code you've written, the better your mark is likely to be.