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

Fix crash when dxCreateTexture higher than 4096 pixels #2737

Closed
wants to merge 4 commits into from
Closed

Fix crash when dxCreateTexture higher than 4096 pixels #2737

wants to merge 4 commits into from

Conversation

Allerek
Copy link
Contributor

@Allerek Allerek commented Sep 12, 2022

Should fix #287 according to

When it comes to operations with textures MTA is relying on D3D9 API. But the largest texture size is 4096 × 4096 in DX9. Of course you can try to create a bigger texture but it's unsafe and can result in unpredictable consequences.

Test resource
texture_test.zip

@Lpsd
Copy link
Member

Lpsd commented Sep 12, 2022

Assuming you've forgotten to uncomment the code?

Also, the wording should be Expected number less than, or equal to 4096 at argument 1 since you're comparing as x > 4096

@Allerek
Copy link
Contributor Author

Allerek commented Sep 12, 2022

Assuming you've forgotten to uncomment the code?

Also, the wording should be Expected number <less/greater> than, or equal to 4096 at argument 1 since you're comparing as x > 4096

Hmm, github desktop automatically commented code out, weird.

Copy link
Member

@Lpsd Lpsd left a comment

Choose a reason for hiding this comment

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

LGTM

@lopezloo lopezloo added the bug Something isn't working label Sep 12, 2022
@samr46
Copy link
Contributor

samr46 commented Sep 12, 2022

This will break current behavior of dxCreateTexture.

Also as far as I know it crashes (in really rare cases) only if setPixels is used. So if you wanna block anything higher than 4096 (in my opinion a bad idea) then it makes sense to patch setPixels leaving dxCreateTexture unchanged. Or better just don't patch anything and put a giant warning on wiki about dimensions higher than 4096.

@Lpsd
Copy link
Member

Lpsd commented Sep 12, 2022

We shouldn't allow textures larger than the API specification regardless

@CrosRoad95
Copy link
Contributor

CrosRoad95 commented Sep 12, 2022

e? people are using bigger textures for radar, does it break backward compatibility?

@Allerek
Copy link
Contributor Author

Allerek commented Sep 12, 2022

This will break current behavior of dxCreateTexture.

Also as far as I know it crashes (in really rare cases) only if setPixels is used. So if you wanna block anything higher than 4096 (in my opinion a bad idea) then it makes sense to patch setPixels leaving dxCreateTexture unchanged. Or better just don't patch anything and put a giant warning on wiki about dimensions higher than 4096.

If you check test resource and code, you will notice it only blocks it when creating textures by giving pixels, using image that have even 20kx20k is fine.

No idea why, and how it works - but it does.

@Lpsd
Copy link
Member

Lpsd commented Sep 12, 2022

possibly. but I feel like we often make decisions based on bad practices in existing scripts and being scared of breaking those. We really should care less if someone's spaghetti script breaks in a major release (1.6) due to beneficial changes (conforming to API specifications) and they are unable to access original source or don't have the knowledge to upgrade it.

Let's consider this for 1.6

You really don't need to be creating textures larger than 4096, and if for some reason you do need textures larger than this you can just use multiple textures...

@Allerek
Copy link
Contributor Author

Allerek commented Sep 12, 2022

e? people are using bigger textures for radar, does it break backward compatibility?

Breaking BWC for crash fixes was done before a lot of times, i think its ok to do this.

@Lpsd
Copy link
Member

Lpsd commented Sep 12, 2022

Or better just don't patch anything and put a giant warning on wiki about dimensions higher than 4096.

This does precisely nothing. See MySQL module

@Lpsd
Copy link
Member

Lpsd commented Sep 12, 2022

See discussion on MTA development discord (just search for 4096, lol) - I don't agree now to clamp this, as the underlying issue is more likely related to setPixel itself so we're just covering up the issue here.

The texture size limit comes from GPU, not the DX9 specification (100% sure it used to be in the specification by Microsoft, but later removed).

We need to investigate #287 properly

@Lpsd Lpsd closed this Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashes with dxCreateTexture with pixel count+:setPixels()
5 participants