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
VideoCommon/TextureInfo: Fix mipmap loading from tmem #9826
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Thanks Techjar!
| @@ -86,6 +86,7 @@ TextureInfo::TextureInfo(const u8* ptr, const u8* tlut_ptr, u32 address, | |||
|
|
|||
| // load mips - TODO: Loading mipmaps from tmem is untested! | |||
| const u8* src_data = m_ptr + GetTextureSize(); | |||
| tmem_even += m_texture_size; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only do this if tmem_even is not a nullptr, otherwise, it seems like this has UB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good idea as a matter of principle, though it doesn't technically matter since we don't dereference in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was a bit too quick I think? I believe the ptr needs to be checked for nullptr
|
| @@ -86,6 +86,8 @@ TextureInfo::TextureInfo(const u8* ptr, const u8* tlut_ptr, u32 address, | |||
|
|
|||
| // load mips - TODO: Loading mipmaps from tmem is untested! | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this comment be modified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno, I didn't put it there. Unsure whether it's still relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this counts as testing it, since the fact this fixes the issue proves that the game is loading mipmaps from tmem (and that we're doing so correctly, bug notwithstanding).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was an old comment from the previous code, no idea why it was there. I just left it in when I ported it over.
Even address needs to be offset to the first mipmap entry.
Even address needs to be offset to the first mipmap entry.
Fixes https://bugs.dolphin-emu.org/issues/12556