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

[d3d9] fix opCompositeExtract out of bound #3390

Merged

Conversation

AlexBozhenk
Copy link
Contributor

Closes: #3293

Signed-by: Oleksii Bozhenko oleksii.bozhenko@globallogic.com

@DadSchoorse
Copy link
Contributor

Can you explain why this is nessecary? Fixed function code isn't easy to understand.

The commit message should be [d3d9] ... instead of spirv: ..., by the way.

@AlexBozhenk AlexBozhenk force-pushed the spirv/fix_opCompositeExtract branch from 7a9ea84 to 1016177 Compare May 3, 2023 10:21
@AlexBozhenk AlexBozhenk changed the title spirv: fix opCompositeExtract out of bound [d3d9] fix opCompositeExtract out of bound May 3, 2023
@AlexBozhenk
Copy link
Contributor Author

AlexBozhenk commented May 3, 2023

@DadSchoorse Yep! As mentioned in #3293 (comment) we have out of bound during compilation to spirv. Disassembling the shader shows that the var is float[4]

%26 = OpCompositeConstruct %v4float %23 %24 %float_0 %float_0
...
%48 = OpLoad %v4float %in_Texcoord0
%49 = OpSelect %v4float %45 %26 %48
...
%108 = OpCompositeExtract %float %49 6

In the fixed function we trying to get the last element. So, all I've changed relates to array size.

@@ -1115,6 +1115,7 @@ namespace dxvk {
default:
case (DXVK_TSS_TCI_PASSTHRU >> TCIOffset):
transformed = m_vs.in.TEXCOORD[inputIndex & 0xFF];
count = 4;
Copy link
Collaborator

@misyltoad misyltoad May 3, 2023

Choose a reason for hiding this comment

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

This is wrong, TCI_PASSTHRU doesn't set count = 4 because it's meant to use the size of the texcoord passed in.

Is this required to close the other issue? If so I imagine there is something else going on, but this is definitely not the right solution!

https://learn.microsoft.com/en-us/windows/win32/direct3d9/d3dtexturetransformflags

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you hitting a case here where count == 0? Curious... That might be the D3DTTFF_DISABLE case that is missing some handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. I'm hitting count == 7. Which is more than texcoord size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For case if we have count smaller than 4 can suggest this:

Suggested change
count = 4;
count = std::min(count, static_cast<uint32_t>(4));

Copy link
Contributor Author

@AlexBozhenk AlexBozhenk May 4, 2023

Choose a reason for hiding this comment

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

one more way is to prevent D3DTTFF_DISABLE if we have count >= 4 here. Obviously, it doesn't make sense to do opCompositeExtract if we don't opCompositeInsert(which won't do if count >= 4)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

count == 7 && texcoordCount == 0
Good for you ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If texcoordCount == 0, we probably want to treat it as NINED3DTSS_TCI_DISABLE right?

Copy link
Contributor Author

@AlexBozhenk AlexBozhenk May 5, 2023

Choose a reason for hiding this comment

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

Sry, what is NINED3DTSS_TCI_DISABLE ?
are you talking about D3DTTFF_DISABLE?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I had a look at what Gallium Nine (see link above) did and it disabled it in that case. So I imagine that's probably what we should do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Pls, check if it's what you meant :)

@misyltoad
Copy link
Collaborator

Once we solve what is going on with that random count = 4, I am happy to merge this.

@AlexBozhenk AlexBozhenk force-pushed the spirv/fix_opCompositeExtract branch 2 times, most recently from f3f2a2f to e0847b9 Compare May 4, 2023 12:07
Closes: doitsujin#3293

Signed-by: Oleksii Bozhenko <oleksii.bozhenko@globallogic.com>
@AlexBozhenk AlexBozhenk force-pushed the spirv/fix_opCompositeExtract branch from e0847b9 to e833072 Compare May 8, 2023 16:21
@misyltoad misyltoad merged commit 80f7d2a into doitsujin:master May 9, 2023
@AlexBozhenk AlexBozhenk deleted the spirv/fix_opCompositeExtract branch May 10, 2023 06:44
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.

Cold fear dont render correctly (light issue?)
3 participants