Skip to content

Conversation

@rtarun9
Copy link
Contributor

@rtarun9 rtarun9 commented Oct 24, 2022

Use row major matrices rather than the default column major matrices in the vertex shaders. Also fixed incorrect window title names in the RasterizerState(1-3-5) and Camera(1-3-6) chapter source codes.

…in the vertex shaders. Also fixed incorrect window title names in the Camera(1-3-6) and Rasterizer State(1-3-5) chapter source codes.
Copy link
Contributor

@deccer deccer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi tarun, thank you for those fixes :)

Did you by any chance also check any mark down files where we might have hlsl code to explain things as well?

@rtarun9
Copy link
Contributor Author

rtarun9 commented Oct 24, 2022

We could mention the reason of using row_major matrices in the markdown files for chapter 1-1-3-hello-triangle, as it mentions this:

To do this we will use DirectXMath which has types that map perfectly to HLSL, both of our inputs are float3 in HLSL, which means that this translates to DirectX::XMFLOAT3.

None of the chapters that actually use matrices have docs ready for them yet.
Should I add a note about DirectX::XMMatrix / DirectX::XMFLOAT4x4 beneath the notes on vector?

@deccer
Copy link
Contributor

deccer commented Oct 24, 2022

I think that's a good idea. Yes please.

@deccer
Copy link
Contributor

deccer commented Oct 25, 2022

OK, as discussed on discord, lets keep the XMMATRIX block of explanation for future chapters where matrices are actually used. This PR shall just fix the dissonance between column/row majorisms

Copy link
Contributor

@deccer deccer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@deccer deccer merged commit 97d7796 into GraphicsProgramming:main Oct 25, 2022
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

Successfully merging this pull request may close these issues.

2 participants