EngoEngine / engo

Engo is an open-source 2D game engine written in Go.
https://engoengine.github.io
MIT License
1.75k stars 136 forks source link

Fix filters #680

Closed inkeliz closed 5 years ago

inkeliz commented 5 years ago

That change is intended to fix https://github.com/EngoEngine/engo/issues/679. The default value of lastMagFilter and lastMinFilter are equal to FilterNearest, which seems never to apply that filter.

That changes set a default value to 255 (uint8 maximum value), which will be not equal to 0 by default:

s.lastMagFilter = 255
s.lastMinFilter = 255

However, any ZoomFilter still FilterNearest by default (since it still 0), any existing code still work without any change.


We can probably add the 255 as a new ZoomFilter at render.go:

const (
   // FilterNearest is a simple nearest neighbor algorithm
   FilterNearest ZoomFilter = iota
   // FilterLinear is a bilinear interpolation algorithm
   FilterLinear
   // filterNone is a default value for render functions
   filterNone ZoomFilter = 255
)

I try to make changes as minimum as possible, so I choose not to create a new constant. Maybe have a better fix for that issue.

Noofbiz commented 5 years ago

The change is great. I missed this bug when testing the filters, but looking back it's exactly what caused a few issues with them lol. Thanks for the fix! I'll work on the CI stuff and get it in ^_^

inkeliz commented 5 years ago

I'm still testing some stuff and trying to understand it. Seems that lastMagFilter and lastMinFilter is never updated. I imagine that something like the code below was suppose to exist:

if s.lastMagFilter != ren.magFilter {
   //...
   s.lastMagFilter = ren.magFilter
}

The current code will be always 255. However, if that is suppose to update, we need to reset the value when change the texture:

if s.lastTexture != ren.Drawable.Texture() {
    s.flush()
    engo.Gl.BindTexture(engo.Gl.TEXTURE_2D, ren.Drawable.Texture())
    s.lastTexture = ren.Drawable.Texture()
    s.lastMinFilter = 255 // << Here
    s.lastMagFilter = 255  // << Here

I thought that create a new function, like func (s *basicShader) resetLasts(), maybe is the way to go. That function can reset the value of last*.


I notice that my change cause a significant performance impact, from 2% to 5.5% of GPU usage. I remove the s.flush() inside each if s.lastRepeating != ren.Repeat, if s.lastMagFilter != ren.magFilter and s.lastMinFilter != ren.minFilter and that brings back to 2%. :\

I don't understand anything about OpenGL, or graphical stuff, so that can be wrong. But, I don't see the point of the flush here, seems that the filter will change on the fly: during the render. I don't know, I need to sleep now. :P

inkeliz commented 5 years ago

The first change causes the Engo to call engo.Gl.TexParameteri and s.flush() everytime, since the lastRepeating, lastMagFilter and lastMinFilter was never updated. The if s.lastMagFilter != ren.magFilter is always true.

That new change adds a new s.lastMagFilter = ren.magFilter and so on, reducing calls to OpenGL. I keep the s.flush(), but it only applies if the filter is different from the previous one, as expected.

I create a new function setTexture to set all s.last* stuff to 255.

Noofbiz commented 5 years ago

Sorry it's taking so long to get this properly reviewed and put in. I've been having a combination of personal and medical issues this year which has really limited my free time. It looks good looking it over so I'll try to get it in hopefully by tomorrow.