Update gl.cpp #2

ricosolana · 2021-02-09T18:32:23Z

Support for SDL2, and fixed some divide by 0 errors with Fix<>, changed some other things for simplicity.

Some questions:

why use this to create the depth buffer instead of new:
z_buffer = new std::remove_reference<decltype(*z_buffer)>::type[SCREEN_WIDTH*SCREEN_HEIGHT];
why use <signal.h>?
why were all the beginning variables static?
lastly, the #define cluster around line 477, why so much/complicated?:

//I hate code duplication more than macros and includes
#ifdef TEXTURE_SUPPORT
    #define TRANSPARENCY
    #include "triangle.inc.h"
    #undef TRANSPARENCY
    #undef TEXTURE_SUPPORT
    #define FORCE_COLOR
    #include "triangle.inc.h"
    #define TEXTURE_SUPPORT
    #undef FORCE_COLOR
#endif
#include "triangle.inc.h"

A lot of the code will appear as changed because MSVC beautified it. I spent a lot of time wondering why I couldn't get a triangle to draw, so I went ahead to learn about matrices and stuff to try and backtrack the problem. I thought of enabling wireframe, so it would draw a 3d line which was pretty basic, then I got the div by 0 errors... Well it's fixed now, I hope my changes can be used.

Vogtinator · 2021-02-09T19:14:39Z

Yay!

Support for SDL2, and fixed some divide by 0 errors with Fix<>, changed some other things for simplicity.

Please split all that into separate commits, especially the reformatting. I see that there are various changes which aren't just reformatting but also change the meaning of code (e.g. replacing decltype with float or removing static), those have to be undone.

It also changed e.g const VERTEX *v1 to const VERTEX* v1, which is a regression. It makes it clearer that with VERTEX *v1, v2; for instance v2 is not a pointer.

Some questions:

why use this to create the depth buffer instead of new:
z_buffer = new std::remove_reference<decltype(*z_buffer)>::type[SCREEN_WIDTH*SCREEN_HEIGHT];

At some point I was experimenting with using different precisions for the Z buffer, so I used this way of getting the right type without having to change it everywhere. Meanwhile it could be replaced by hardcoding GLFix though.

why use <signal.h>?

It's used for installing a SIGINT handler to quit SDL. Without that, you can't terminate the program properly. I guess MSVC doesn't implement this? In that case, you can wrap it in #ifndef __WIN32__ or similar.

why were all the beginning variables static?

They're not supposed to be visible externally/globally, which also allows for more optimizations. See also https://stackoverflow.com/questions/1856599/when-to-use-static-keyword-before-global-variables for more info.

lastly, the #define cluster around line 477, why so much/complicated?:

triangle.inc.h contains just a single function for drawing triangles, but it has several different modes which require different code (interpolating U/V coords, transparency, combination thereof, etc.). Doing that with plain if(transparency) or such would be too slow, so it uses the preprocessor instead to have separate functions without having to duplicate all the code manually.

//I hate code duplication more than macros and includes
#ifdef TEXTURE_SUPPORT
    #define TRANSPARENCY
    #include "triangle.inc.h"
    #undef TRANSPARENCY
    #undef TEXTURE_SUPPORT
    #define FORCE_COLOR
    #include "triangle.inc.h"
    #define TEXTURE_SUPPORT
    #undef FORCE_COLOR
#endif
#include "triangle.inc.h"
A lot of the code will appear as changed because MSVC beautified it. I spent a lot of time wondering why I couldn't get a triangle to draw, so I went ahead to learn about matrices and stuff to try and backtrack the problem. I thought of enabling wireframe, so it would draw a 3d line which was pretty basic, then I got the div by 0 errors... Well it's fixed now, I hope my changes can be used.

Yeah, the line drawing code wasn't really used beyond the initial bringup, it's older than the triangle drawing code and uses a naive, unoptimized and broken way of drawing.

I'll leave some comments about the code changes in the review section.

gl.cpp

This reverts commit 0c26fb3.

This reverts commit b53c81a.

new ABS() method for GLFix could be changed to something else if some more performance could be squeezed

not really necessary but heck

This reverts commit 8195e1d.

ricosolana · 2021-02-10T02:35:41Z

Ok I made the split commits, all organized now.
For some reason when I disable WIREFRAME, nothing gets rendered.

ricosolana · 2021-02-10T02:51:57Z

I just realized something about the transformation matrices, not all of the nodes are used for translations/rotations/scaling,
. In the nglMultMatMat, and maybe nglMultMatVectRes functions could have those specific indexes removed from the operations since it will be equivalent to adding 0.
I don't know if this will make a difference but it seems like something that could be trimmed for performance.

ricosolana · 2021-02-11T17:31:11Z

__builtin_expect macro for gnu isn't recognized by msvc, so it doesn't work as expected when I add the custom macro #define __builtin_expect(exp, c) exp != c. I'm not sure whether this is correct either.

Vogtinator · 2021-02-24T18:52:44Z

I just realized something about the transformation matrices, not all of the nodes are used for translations/rotations/scaling,
. In the nglMultMatMat, and maybe nglMultMatVectRes functions could have those specific indexes removed from the operations since it will be equivalent to adding 0.
I don't know if this will make a difference but it seems like something that could be trimmed for performance.

You noticed that correctly. The unused part is for the W coordinate, which isn't really needed for 3D affine transformations which always have 0 0 0 1 as last column. Except for the perspective calculation, all transformations in nGL so far are affine, and so this can be optimized away. Vectors are stored as struct VECTOR3 without the W component and it's always treated as 1 in calculations. You can see that for instance in nglMultMatVectRes: There is no *w for the last column. https://en.wikipedia.org/wiki/Transformation_matrix#Affine_transformations explains this approach as well.

This makes nglMultMatVectRes unsuitable for non-affine transformations though, and so perspective calculation in nglPerspective is not done using a projection matrix, but rather manually. That way also some additional calculations can be avoided which aren't needed for this projection.

A optimization similar to VECTOR3 could be done to struct MATRIX by not storing the last column and treating it as 0 0 0 1 and ignoring writes. That makes MATRIX less general-purpose though and wouldn't actually be a huge benefit: Matrix multiplication is only performed to change the world transformation , e.g. by calling glScale3f/glTranslatef/nglRotateX. That doesn't happen often during a frame, is independent of the total vertex/primitive count and doesn't take significant frame time.

Much more time is spent during drawing of primitives, so avoiding overdraw (writing pixels which aren't visible in the final frame) would have a much bigger impact on performance. nGL can't do any visibility calculations depending on scenery, so that's up to the application.

Another topic is that currently nGL uses a 32-bit Z buffer, which has to be read (and written, if in front) for each pixel of a triangle. A different approach is to depth-sort primitives before drawing them, which avoids that access, but increases overdraw as everything behind it which is just partially visible is fully drawn and then just overwritten again.

Vogtinator · 2021-02-24T18:53:42Z

__builtin_expect macro for gnu isn't recognized by msvc, so it doesn't work as expected when I add the custom macro #define __builtin_expect(exp, c) exp != c. I'm not sure whether this is correct either.

It's not - __builtin_expect is to tell the compiler what the "most likely" result of the expression is. So the correct replacement would be just #define __builtin_expect(exp, c) (exp)`.

Vogtinator · 2021-02-24T18:55:18Z

gl.cpp

-#include <signal.h>
-static SDL_Surface *scr;
+#ifdef _WIN32
+    #include <SDL.h>


#include <SDL.h> should actually work for both

Vogtinator · 2021-02-24T18:55:41Z

gl.cpp

-static SDL_Surface *scr;
+#ifdef _WIN32
+    #include <SDL.h>
+    #include <signal.h>


Shouldn't this be in the "not windows" block?

Vogtinator · 2021-02-24T18:56:06Z

gl.cpp

+#else
+    #include <SDL/SDL.h>
+#endif
+SDL_Window* sdl_window;


Should all be static, as not used/accessible outside of this source file

Vogtinator · 2021-02-24T18:56:19Z

gl.cpp

+            SDL_TEXTUREACCESS_STREAMING,
+            SCREEN_WIDTH, SCREEN_HEIGHT);
+
+        SDL_SetWindowTitle(sdl_window, "nGl");


Suggested change

SDL_SetWindowTitle(sdl_window, "nGl");

SDL_SetWindowTitle(sdl_window, "nGL");

Vogtinator · 2021-02-24T18:57:26Z

gl.cpp

@@ -84,7 +100,7 @@ void nglUninit()
    #ifdef _TINSPIRE
        lcd_init(SCR_TYPE_INVALID);
    #else
-        //TODO
+        SDL_DestroyRenderer(sdl_renderer);


Isn't there something like SDL_Quit?

Vogtinator · 2021-02-24T18:59:29Z

gl.cpp

 //Doesn't interpolate colors even if enabled
 void nglDrawLine3D(const VERTEX *v1, const VERTEX *v2)
 {    
-    //TODO: Z-Clipping
+    // Z-Clipping!


Proper Z clipping would involve only drawing the part of the line which fits into the view (z between NEAR_PLANE and inf). This is currently just a band-aid to avoid a division by zero, so the //TODO comment should stay.

Vogtinator · 2021-02-24T19:01:01Z

gl.cpp

-    if(dy > GLFix(1) || dy < GLFix(-1))
+    //if(dy > GLFix(1) || dy < GLFix(-1))
+
+    // if check passes diff_y should always be non zero


You can be more confident here:

Suggested change

// if check passes diff_y should always be non zero

// if check passes, diff_y is non zero

Vogtinator · 2021-02-24T19:06:15Z

gl.cpp

@@ -354,29 +370,36 @@ GLFix nglZBufferAt(const unsigned int x, const unsigned int y)
    return z_buffer[x + y * SCREEN_WIDTH];
 }

+constexpr GLFix ABS(GLFix a) {


This belongs into fix.h, as method

Vogtinator · 2021-02-24T19:12:23Z

gl.cpp

-        const GLFix diff_y = v2_p.y - v1_p.y;
-
-        const GLFix dx = (v2_p.x - v1_p.x) / diff_y;
+        const GLFix dx = diff_x / diff_y;


I don't think it's correct to use those values after the std::swap above. The swap is there to guarantee that v1_p has a lower Y than v2_p, so you would have to change the sign of all the diff_ values as well. Otherwise drawn lines face the wrong direction.

It's easier to just always compute the diffs after the swap though - subtraction is cheap.

Using diff_x / diff_y is fine here. In the case where the points have to be swapped, diff_y is negative and the division returns the correct result. It's important to not mix diffs before and after the swap though, which is the case in line 403 for instance.

Update gl.cpp

0c26fb3

Vogtinator requested changes Feb 9, 2021

View reviewed changes

gl.cpp Outdated Show resolved Hide resolved

gl.cpp Outdated Show resolved Hide resolved

gl.cpp Show resolved Hide resolved

gl.cpp Outdated Show resolved Hide resolved

ricosolana added 10 commits February 9, 2021 20:05

Revert "Update gl.cpp"

179bc07

This reverts commit 0c26fb3.

signal

9f3b46c

SDL2

a01369f

nglDrawLine3D div by 0

b53c81a

Revert "nglDrawLine3D div by 0"

9ac3f0c

This reverts commit b53c81a.

nglDrawLine3D div by 0

8e2e440

new ABS() method for GLFix could be changed to something else if some more performance could be squeezed

forgot abs method

c0d23ed

msvc beautify

8195e1d

not really necessary but heck

Revert "msvc beautify"

1810fa1

This reverts commit 8195e1d.

nglDrawLine3D z-clipping

ed7ac6a

ptr

ca935a2

Vogtinator requested changes Feb 24, 2021

View reviewed changes

ricosolana added 7 commits March 1, 2021 08:23

__builtin_expect for windows

a66153f

static no global

3b41ceb

asserts for sdl

6d897bd

etiquette and sdl_quit

15aced6

screen inverted nspire only

f30880e

added abs to fix.h

93eb162

signal def guard

4d149bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update gl.cpp #2

Update gl.cpp #2

ricosolana commented Feb 9, 2021

Vogtinator commented Feb 9, 2021

ricosolana commented Feb 10, 2021

ricosolana commented Feb 10, 2021

ricosolana commented Feb 11, 2021

Vogtinator commented Feb 24, 2021

Vogtinator commented Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

Vogtinator Feb 24, 2021

	SDL_SetWindowTitle(sdl_window, "nGl");
	SDL_SetWindowTitle(sdl_window, "nGL");

	// if check passes diff_y should always be non zero
	// if check passes, diff_y is non zero

Update gl.cpp #2

Are you sure you want to change the base?

Update gl.cpp #2

Conversation

ricosolana commented Feb 9, 2021

Vogtinator commented Feb 9, 2021

ricosolana commented Feb 10, 2021

ricosolana commented Feb 10, 2021

ricosolana commented Feb 11, 2021

Vogtinator commented Feb 24, 2021

Vogtinator commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment