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

Implemented support for mipmap generation #973

Merged
merged 1 commit into from May 6, 2016

Conversation

Projects
None yet
5 participants
@binary1248
Member

binary1248 commented Sep 23, 2015

This is the GL_EXT_framebuffer_object variant that relies on explicit generation by the user rather than implicit generation every time the base level image is modified.

Proper support for sf::RenderTexture has also been implemented.

Supersedes #498, implements #123.

@binary1248 binary1248 self-assigned this Sep 23, 2015

@binary1248 binary1248 added this to the 2.4 milestone Sep 23, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 23, 2015

Member

Looks good overall.

Some remarks:

  • Do we really need to expose a hasMipMap() function? I can't see any useful use of such a function (regarding how sf::RenderTexture uses it: the check could be done inside invalidateMipmap() so the mipmap state could be kept private inside sf::Texture)?
  • Do we really want to remove manual texture management from the OpenGL example? The point of this example is to show interoperability between SFML and OpenGL.
  • Can't we invalidate the mipmaps inside sf::RenderTexture::display(), to avoid making the draw function virtual?
  • Shouldn't we invalidate the mipmap automatically in loadFromXxx and update?
  • I would also explain what mipmap is and in which circumstances it is used by the graphics card, in the function and/or class documentation.
Member

LaurentGomila commented Sep 23, 2015

Looks good overall.

Some remarks:

  • Do we really need to expose a hasMipMap() function? I can't see any useful use of such a function (regarding how sf::RenderTexture uses it: the check could be done inside invalidateMipmap() so the mipmap state could be kept private inside sf::Texture)?
  • Do we really want to remove manual texture management from the OpenGL example? The point of this example is to show interoperability between SFML and OpenGL.
  • Can't we invalidate the mipmaps inside sf::RenderTexture::display(), to avoid making the draw function virtual?
  • Shouldn't we invalidate the mipmap automatically in loadFromXxx and update?
  • I would also explain what mipmap is and in which circumstances it is used by the graphics card, in the function and/or class documentation.
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 23, 2015

Member

Do we really need to expose a hasMipMap() function? I can't see any useful use of such a function (regarding how sf::RenderTexture uses it: the check could be done inside invalidateMipmap() so the mipmap state could be kept private inside sf::Texture)?

While hasMipmap() is not really necessary, it might be convenient for the user if their architecture requires them to be able to tell if mipmaps need to be regenerated or not. If we don't track this state, they will have to themselves, and I assume it is simpler for us to do directly in sf::Texture than it is for them to implement in some auxiliary class.

Also, invalidateMipmap() is a private function at the moment, only meant to be used by RenderTexture (which is already a friend class). I don't see any reason to make it public if we already make sure any modification by the user results in proper invalidation.

Do we really want to remove manual texture management from the OpenGL example? The point of this example is to show interoperability between SFML and OpenGL.

From what I can tell (you can see this in the comment which I forgot to change when I removed GLU), the only reason for the manual management in that example was because we wanted to use GLU to generate the mipmap. Since GLU got removed, and we can even generate the mipmap ourselves now, there is no point to manually manage texture creation. And besides... this is a better example of interoperability between SFML and OpenGL if you ask me. 😉

Can't we invalidate the mipmaps inside sf::RenderTexture::display(), to avoid making the draw function virtual?

Well... it depends on what we tell users who don't bother calling sf::RenderTexture::display() after they are done drawing. The main problem is that the behaviour varies based on what implementation is used. When using the default implementation, the texture will really only be updated once display() is called and not before. When using the FBO implementation, calling display() is not necessary if you are lucky since the texture is directly modified in every draw. You can call it an error to not call display() even if it isn't necessary in this case, but invalidating in draw() ensures consistent behaviour regardless of which implementation is used.

What makes it even worse is that the current mipmap implementation doesn't even work unless the FBO implementation is available in the first place, so the case where it would actually make sense (the default implementation) doesn't even happen in practice.

Shouldn't we invalidate the mipmap automatically in loadFromXxx and update?

It does get invalidated. I just omitted the additional texture saving etc. that is present in invalidateMipmap() and inlined the 2 lines that actually perform the "invalidation". Like I said above, invalidateMipmap() is only meant for RenderTexture, that's why we need to do a bit more in there.

Member

binary1248 commented Sep 23, 2015

Do we really need to expose a hasMipMap() function? I can't see any useful use of such a function (regarding how sf::RenderTexture uses it: the check could be done inside invalidateMipmap() so the mipmap state could be kept private inside sf::Texture)?

While hasMipmap() is not really necessary, it might be convenient for the user if their architecture requires them to be able to tell if mipmaps need to be regenerated or not. If we don't track this state, they will have to themselves, and I assume it is simpler for us to do directly in sf::Texture than it is for them to implement in some auxiliary class.

Also, invalidateMipmap() is a private function at the moment, only meant to be used by RenderTexture (which is already a friend class). I don't see any reason to make it public if we already make sure any modification by the user results in proper invalidation.

Do we really want to remove manual texture management from the OpenGL example? The point of this example is to show interoperability between SFML and OpenGL.

From what I can tell (you can see this in the comment which I forgot to change when I removed GLU), the only reason for the manual management in that example was because we wanted to use GLU to generate the mipmap. Since GLU got removed, and we can even generate the mipmap ourselves now, there is no point to manually manage texture creation. And besides... this is a better example of interoperability between SFML and OpenGL if you ask me. 😉

Can't we invalidate the mipmaps inside sf::RenderTexture::display(), to avoid making the draw function virtual?

Well... it depends on what we tell users who don't bother calling sf::RenderTexture::display() after they are done drawing. The main problem is that the behaviour varies based on what implementation is used. When using the default implementation, the texture will really only be updated once display() is called and not before. When using the FBO implementation, calling display() is not necessary if you are lucky since the texture is directly modified in every draw. You can call it an error to not call display() even if it isn't necessary in this case, but invalidating in draw() ensures consistent behaviour regardless of which implementation is used.

What makes it even worse is that the current mipmap implementation doesn't even work unless the FBO implementation is available in the first place, so the case where it would actually make sense (the default implementation) doesn't even happen in practice.

Shouldn't we invalidate the mipmap automatically in loadFromXxx and update?

It does get invalidated. I just omitted the additional texture saving etc. that is present in invalidateMipmap() and inlined the 2 lines that actually perform the "invalidation". Like I said above, invalidateMipmap() is only meant for RenderTexture, that's why we need to do a bit more in there.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 23, 2015

Member

While hasMipmap() is not really necessary, it might be convenient for the user if their architecture requires them to be able to tell if mipmaps need to be regenerated or not. If we don't track this state, they will have to themselves, and I assume it is simpler for us to do directly in sf::Texture than it is for them to implement in some auxiliary class.

Not sure. In most scenarios, users will know exactly when to regenerate mipmaps without tracking a state (after the call to load or update, or every frame). I'd really prefer to go with the minimal API first, and add this kind of function (which is pure convenience) later if really needed.

this is a better example of interoperability between SFML and OpenGL if you ask me

I thought about it again and reached the same conclusion. I even knew that you would answer that 😛

Well... it depends on what we tell users who don't bother calling sf::RenderTexture::display() after they are done drawing

So far, we have always enforced the call to display() on render textures. Not calling it could be considered a UB. I don't like half-defined states, or assertions that are verified only under some environments and not some others. The API is well-defined, the fact that it works sometimes without it is only a side-effect. This also leaves us more room for implementation -- like in this very specific case. We don't break anything if we do it in display(), so I don't see why we shouldn't do it. Given the "ugly" modification required to the API, I think it's worth it.

It does get invalidated

Sorry, I missed that in the diff.

And don't forget my last point, I guess I added it after you read my post:

I would also explain what mipmap is and in which circumstances it is used by the graphics card, in the function and/or class documentation.

Member

LaurentGomila commented Sep 23, 2015

While hasMipmap() is not really necessary, it might be convenient for the user if their architecture requires them to be able to tell if mipmaps need to be regenerated or not. If we don't track this state, they will have to themselves, and I assume it is simpler for us to do directly in sf::Texture than it is for them to implement in some auxiliary class.

Not sure. In most scenarios, users will know exactly when to regenerate mipmaps without tracking a state (after the call to load or update, or every frame). I'd really prefer to go with the minimal API first, and add this kind of function (which is pure convenience) later if really needed.

this is a better example of interoperability between SFML and OpenGL if you ask me

I thought about it again and reached the same conclusion. I even knew that you would answer that 😛

Well... it depends on what we tell users who don't bother calling sf::RenderTexture::display() after they are done drawing

So far, we have always enforced the call to display() on render textures. Not calling it could be considered a UB. I don't like half-defined states, or assertions that are verified only under some environments and not some others. The API is well-defined, the fact that it works sometimes without it is only a side-effect. This also leaves us more room for implementation -- like in this very specific case. We don't break anything if we do it in display(), so I don't see why we shouldn't do it. Given the "ugly" modification required to the API, I think it's worth it.

It does get invalidated

Sorry, I missed that in the diff.

And don't forget my last point, I guess I added it after you read my post:

I would also explain what mipmap is and in which circumstances it is used by the graphics card, in the function and/or class documentation.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 23, 2015

Member

The issues should be fixed now.

Member

binary1248 commented Sep 23, 2015

The issues should be fixed now.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 24, 2015

Member

👍

Does this need more testing on OpenGL ES platforms?

Member

LaurentGomila commented Sep 24, 2015

👍

Does this need more testing on OpenGL ES platforms?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 2, 2015

Member

Rebased and added missing define for OpenGL ES platforms.

Could use some OpenGL ES testing. @MarioLiebisch, @BlueCobold

Member

binary1248 commented Oct 2, 2015

Rebased and added missing define for OpenGL ES platforms.

Could use some OpenGL ES testing. @MarioLiebisch, @BlueCobold

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Oct 2, 2015

Contributor

Will see if I can find some time this weekend to check for iOS.

Contributor

BlueCobold commented Oct 2, 2015

Will see if I can find some time this weekend to check for iOS.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 15, 2016

Member

Bump.

Member

binary1248 commented Jan 15, 2016

Bump.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 26, 2016

Member

Working flawlessly on Arch Linux. 👍 🐈

Member

TankOs commented Jan 26, 2016

Working flawlessly on Arch Linux. 👍 🐈

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 18, 2016

Member

@MarioLiebisch, @BlueCobold Any testing on this?

Member

eXpl0it3r commented Feb 18, 2016

@MarioLiebisch, @BlueCobold Any testing on this?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 4, 2016

Member

Since iOS/Android are still experimental, should we proceed to and merge this?

Member

eXpl0it3r commented Mar 4, 2016

Since iOS/Android are still experimental, should we proceed to and merge this?

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Mar 4, 2016

Contributor

Is there a minimal sample to test this? Then it would be far easier for me to check if it's working - basically just compile the libs and test it.

Contributor

BlueCobold commented Mar 4, 2016

Is there a minimal sample to test this? Then it would be far easier for me to check if it's working - basically just compile the libs and test it.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 4, 2016

Member

The OpenGL example uses the mipmaps, so you could just built these, or does that not work for iOS?

Member

eXpl0it3r commented Mar 4, 2016

The OpenGL example uses the mipmaps, so you could just built these, or does that not work for iOS?

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Mar 4, 2016

Contributor

I'll check.

Contributor

BlueCobold commented Mar 4, 2016

I'll check.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 20, 2016

Member

Any results so far @BlueCobold?

What about @MarioLiebisch?

Member

eXpl0it3r commented Apr 20, 2016

Any results so far @BlueCobold?

What about @MarioLiebisch?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 4, 2016

Member

@binary1248 Could you rebase the PR?

Also could we potentially move on without proper OpenGL ES testing?

Member

eXpl0it3r commented May 4, 2016

@binary1248 Could you rebase the PR?

Also could we potentially move on without proper OpenGL ES testing?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 4, 2016

Member

Done.

Member

binary1248 commented May 4, 2016

Done.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 5, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented May 5, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r added the s:accepted label May 5, 2016

@eXpl0it3r eXpl0it3r merged commit 259811d into master May 6, 2016

14 checks passed

debian-gcc-64 Build #126 done.
Details
freebsd-gcc-64 Build #126 done.
Details
osx-clang-el-capitan Build #8 done.
Details
static-analysis Build #126 done.
Details
windows-gcc-492-tdm-32 Build #11 done.
Details
windows-gcc-492-tdm-64 Build #11 done.
Details
windows-gcc-530-mingw-32 Build #11 done.
Details
windows-gcc-530-mingw-64 Build #11 done.
Details
windows-vc11-32 Build #127 done.
Details
windows-vc11-64 Build #127 done.
Details
windows-vc12-32 Build #127 done.
Details
windows-vc12-64 Build #125 done.
Details
windows-vc14-32 Build #126 done.
Details
windows-vc14-64 Build #128 done.
Details

@eXpl0it3r eXpl0it3r deleted the feature/mipmap branch May 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment