Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple issues in GL↔GU matrix handling #87

Open
I-asked opened this issue Dec 8, 2024 · 7 comments
Open

Multiple issues in GL↔GU matrix handling #87

I-asked opened this issue Dec 8, 2024 · 7 comments

Comments

@I-asked
Copy link

I-asked commented Dec 8, 2024

Bug Report

What's the issue you encountered?

for (int i = 0; i < 16; i++) {
3 * 4 != 16
mv[i%4][i/4] = source[i];
→ OpenGL is column-major, leading to unintended transposition of the matrix

opengx/src/getters.c

Lines 128 to 159 in 60afb21

void glGetFloatv(GLenum pname, GLfloat *params)
{
switch (pname) {
case GL_CURRENT_RASTER_POSITION:
floatcpy(params, glparamstate.raster_pos, 4);
break;
case GL_DEPTH_BIAS:
*params = glparamstate.transfer_depth_bias;
break;
case GL_DEPTH_RANGE:
params[0] = glparamstate.depth_near;
params[1] = glparamstate.depth_far;
break;
case GL_DEPTH_SCALE:
*params = glparamstate.transfer_depth_scale;
break;
case GL_MODELVIEW_MATRIX:
for (int i = 0; i < 3; i++)
for (int j = 0; j < 4; j++)
params[j * 4 + i] = glparamstate.modelview_matrix[i][j];
params[3] = params[7] = params[11] = 0.0f;
params[15] = 1.0f;
return;
case GL_PROJECTION_MATRIX:
for (int i = 0; i < 4; i++)
for (int j = 0; j < 4; j++)
params[j * 4 + i] = glparamstate.projection_matrix[i][j];
return;
default:
return;
};
}
→ GL state not synced; the getter is unaware of the changes made by glFrustum

I tried fixing the aforementioned issues, but sadly, that did not fix all of the matrix-related issues, so there must be something I missed (or misunderstood)…

How can the issue be reproduced?

N/A

Environment?

N/A

Additional context?

Hi there, I'm "porting" the Blender Game Engine to Wii and trying to use OpenGX, but I've encountered multiple issues with the matrix logic.

Also thanks to everyone involved for y'all's work on the project! 😁👍

@mardy
Copy link
Collaborator

mardy commented Dec 8, 2024

Hi Julia, thanks for your report, and it's great to see that you are working on such an interesting project like Blender Game Engine. But does it work with OpenGL 1.x?

Answering to your points:

for (int i = 0; i < 16; i++) {

3 * 4 != 16

Ouch, this is 100% wrong! I guess the only reason why this doesn't cause any visible defect is that the matrix after the current one is anyway always unset, so we get a real memory corruption only if the matrix stack is full. I'll fix this.

mv[i%4][i/4] = source[i];

→ OpenGL is column-major, leading to unintended transposition of the matrix

Yes, but are you sure that this is wrong? GX is row-major, so the conversion seems correct to me.

opengx/src/getters.c

Lines 128 to 159 in 60afb21

void glGetFloatv(GLenum pname, GLfloat *params)
{
switch (pname) {
case GL_CURRENT_RASTER_POSITION:
floatcpy(params, glparamstate.raster_pos, 4);
break;
case GL_DEPTH_BIAS:
*params = glparamstate.transfer_depth_bias;
break;
case GL_DEPTH_RANGE:
params[0] = glparamstate.depth_near;
params[1] = glparamstate.depth_far;
break;
case GL_DEPTH_SCALE:
*params = glparamstate.transfer_depth_scale;
break;
case GL_MODELVIEW_MATRIX:
for (int i = 0; i < 3; i++)
for (int j = 0; j < 4; j++)
params[j * 4 + i] = glparamstate.modelview_matrix[i][j];
params[3] = params[7] = params[11] = 0.0f;
params[15] = 1.0f;
return;
case GL_PROJECTION_MATRIX:
for (int i = 0; i < 4; i++)
for (int j = 0; j < 4; j++)
params[j * 4 + i] = glparamstate.projection_matrix[i][j];
return;
default:
return;
};
}

→ GL state not synced; the getter is unaware of the changes made by glFrustum

Mmm... I don't really see a synchronisation issue here: glFrustum is implemented by means of glMultMatrixf, which write to glparamstate.projection_matrix, so that part should be correct. What strikes me as wrong is the fact that we prepare a Mtx44 (which is row-major) as an input to glMultMatrixf, which expects a GL matrix. So, either this doesn't work, or, if it works, it means that we have yet another mistake somewhere else that compensates this one. :-)

Do you happen to have a snippet of code that can be used to reproduce the problems you see? That would be useful.

mardy added a commit to mardy/opengx that referenced this issue Dec 8, 2024
GX matrices are 3x4, and the old code was overflowing: wee need to skip
the fourth row of the matrix.

Fixes one issue from devkitPro#87
@mardy
Copy link
Collaborator

mardy commented Dec 8, 2024

Oh, I verified now, rebuilding the crack-attack game against the latest opengx, that glFrustum is broken indeed.

@mardy
Copy link
Collaborator

mardy commented Dec 8, 2024

@I-asked, can you please try out #88 ? It fixes the issues for me.

@I-asked
Copy link
Author

I-asked commented Dec 8, 2024

I have a slightly different fix in my set of fixes that I wanted to PR: https://github.com/I-asked/opengx/tree/fixing
This is enough to run my port of BGE on the Wii: https://github.com/I-asked/downbge/tree/porting

@mardy
Copy link
Collaborator

mardy commented Dec 9, 2024

I have a slightly different fix in my set of fixes that I wanted to PR: https://github.com/I-asked/opengx/tree/fixing

Oh, nice, there's some good stuff in there! The matrix changes seem equivalent to mine, for the rest, if/when you prepare a PR, please split the changes into smaller commits, according to the issues they fix.

But those additions of calls to sync the state seem odd, they shouldn't be needed. If they are fixing some issue, it probably means that we have a bug in how we compute the "dirty" flags, and I'd prefer to fix that, instead of adding all those calls :-)

This is enough to run my port of BGE on the Wii: https://github.com/I-asked/downbge/tree/porting

That's amazing! Looking forward to it!

@I-asked
Copy link
Author

I-asked commented Dec 9, 2024

I have a slightly different fix in my set of fixes that I wanted to PR: https://github.com/I-asked/opengx/tree/fixing

Oh, nice, there's some good stuff in there! The matrix changes seem equivalent to mine, for the rest, if/when you prepare a PR, please split the changes into smaller commits, according to the issues they fix.

I will try, definitely! My last commit actually fixes all the bugs I encountered working on downBGE until now.

But those additions of calls to sync the state seem odd, they shouldn't be needed. If they are fixing some issue, it probably means that we have a bug in how we compute the "dirty" flags, and I'd prefer to fix that, instead of adding all those calls :-)

I'll investigate at some point, but for now, they work for me… 🤪

This is enough to run my port of BGE on the Wii: https://github.com/I-asked/downbge/tree/porting

That's amazing! Looking forward to it!

Thank you so much for the kind words! 😊

BTW, I'm actually planning to use opengx in two engines, the other being a fork of Godot 1 that I'm working on with my friend: https://github.com/TheFusionEngine/FusionEngine

@mardy
Copy link
Collaborator

mardy commented Dec 10, 2024

I will try, definitely! My last commit actually fixes all the bugs I encountered working on downBGE until now.

Please base your work on top of #90, since it touches some areas that you might be touching too (the dirty flags, especially!).

BTW, I'm actually planning to use opengx in two engines, the other being a fork of Godot 1 that I'm working on with my friend: https://github.com/TheFusionEngine/FusionEngine

I see dreamcast, vita, so I'm sure it will work nicely for the Wii too :-)

WinterMute pushed a commit that referenced this issue Dec 10, 2024
GX matrices are 3x4, and the old code was overflowing: wee need to skip
the fourth row of the matrix.

Fixes one issue from #87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants