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

Purge calls of GL.GenerateMipmap from Texture constructors #5599

Closed
tomspilman opened this issue Mar 20, 2017 · 9 comments
Closed

Purge calls of GL.GenerateMipmap from Texture constructors #5599

tomspilman opened this issue Mar 20, 2017 · 9 comments
Milestone

Comments

@tomspilman
Copy link
Member

tomspilman commented Mar 20, 2017

In Texture2D in particular it calls GL.GenerateMipmap in some form to fill out the mip map chain.

This is wrong.

GL.GenerateMipmap literally means "generate mip maps from the level 0 data now". It reads each pixel of the level 0 data and filters it down to generate mips. Depending on the target platform on the CPU or the GPU.

It should be painfully obvious that doing this on a empty texture is a waste of time.

Also you cannot call GL.GenerateMipmap on compressed textures as GL will not auto generate mips for compressed formats. It would require GL to compress the texture at runtime which it does not do. So currently this fails for compressed formats.

Versions of MonoGame from just 6 months ago didn't call GL.GenerateMipmap at all:

GL.CompressedTexImage2D(TextureTarget.Texture2D, 0, glInternalFormat,

So either A:

  • We stop calling GL.GenerateMipmap and do nothing for mipmaps on construction (which I don't know is correct).

Or B:

  • We correctly define mip levels on a texture by writing a loop and calling GL.CompressedTexImage2D or GL.TexImage2D for each level.
@tomspilman tomspilman added this to the 3.6.1 Release milestone Mar 20, 2017
@tomspilman
Copy link
Member Author

@Jjagg @KonajuGames

@tomspilman
Copy link
Member Author

I'm actually fairly certain that B is the right path. We need to be calling GL.CompressedTexImage2D/GL.TexImage2D for each level on construction. This ensures a correct texture with the complete mipmap chain is created at construction like we have on all other platforms.

@KonajuGames
Copy link
Contributor

KonajuGames commented Mar 20, 2017 via email

@KakCAT
Copy link
Contributor

KakCAT commented Mar 20, 2017

I think the code to upload mipmaps as you say in 'B' is already working, and all that is needed is removing the Gl.GenerateMipmap call.

A few days ago when I was investigating the issue, removing the call made the mipmaps work, but I was out of time and I couldn't investigate further.

@tomspilman
Copy link
Member Author

I think the code to upload mipmaps as you say in 'B' is already working

No it isn't. There is no loop in the PlatformConstructor code to generate all the mip levels.

https://github.com/MonoGame/MonoGame/blob/develop/MonoGame.Framework/Graphics/Texture2D.OpenGL.cs#L74

This is required to correctly construct a texture with mips.

@KakCAT
Copy link
Contributor

KakCAT commented Mar 20, 2017

I just checked it... the code to read the mipmaps is not in the PlatformConstructor but in the Texture2DReader.cs file.

https://github.com/MonoGame/MonoGame/blob/develop/MonoGame.Framework/Content/ContentReaders/Texture2DReader.cs#L72

https://github.com/MonoGame/MonoGame/blob/develop/MonoGame.Framework/Content/ContentReaders/Texture2DReader.cs#L172

I'm talking about the assets read with the Content object. If you're talking about creating a new Texture2D() then you may be right, I haven't explored that path.

@tomspilman
Copy link
Member Author

the code to read the mipmaps is not in the PlatformConstructor but in the Texture2DReader.cs file.

Sure... I am not speaking of the code to read the data for a mip map from disk and copy it to the texture.

If you're talking about creating a new Texture2D() then you may be right,

Yes I am speaking of that. Specifically...

// On DirectX correctly returns a blank texture with mipmaps!
// On OpenGL incorrectly returns a blank texture without mipmaps!
var tex = new Texture2D(device, 1024, 1024, true, SurfaceFormat.Color);

@tomspilman
Copy link
Member Author

tomspilman commented Mar 20, 2017

Continuing a discussion with @Jjagg from #5557 (comment).

I don't know how to tell GL a texture needs mipmaps without generating them though.

From what I have found you need to call GL.CompressedTexImage2D or GL.TexImage2D for each level in a loop.

Although @KonajuGames has suggested that maybe he did something else to fix this?

@KonajuGames
Copy link
Contributor

#5614 fixed this for Texture2D. Texture3D has no support for mipmaps at this stage. TextureCube still uses glGenerateMipmaps. These could be added as separate issues.

@tomspilman tomspilman modified the milestones: 3.6.1 Release, 3.7 Release Mar 3, 2018
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

No branches or pull requests

3 participants