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

Refactor TRANS_LIT_TEX_VERTEX into two instances #375

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

duncanspumpkin
Copy link
Contributor

_TRANS_LIT_TEX_VERTEX was a problem child in the clang-tidy PR so had a closer look at it. We have two different instances of this in different forms in the codebase. I've created TransformedTexture1Vertex and TransformedTexture2Vertex to replace the two different types. I've also extracted out the D3DFVF values for each struct so its clear why they are being set.

@duncanspumpkin
Copy link
Contributor Author

duncanspumpkin commented Nov 19, 2021

Suggested to use

struct VertexFormatXYZDUV1

Sadly this isn't the same this is for Vector4. Could still use the same naming scheme though.

@duncanspumpkin
Copy link
Contributor Author

We have the following vertex formats defined:
VertexFormatXYZDUV2 within w3dwater.h
VertexFormatXYZNDUV2 within dx8vertexbuffer.h
VertexFormatXYZDUV1 within dx8vertexbuffer.h
This PR adds:
VertexFormatXYZWDUV1 within dx8wrapper.h
VertexFormatXYZWDUV2 within dx8wrapper.h

Copy link
Contributor

@tomsons26 tomsons26 left a comment

Choose a reason for hiding this comment

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

This needs a bit of a debate, it is a bit annoying that these formats are scattered all over the place

@tomsons26 tomsons26 added the investigate Investigation and careful discussion needed label Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Investigation and careful discussion needed refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants