Skip to content

Fix for issue #847, "SamplerStates aren't properly applied when active texture changes"#889

Merged
tomspilman merged 2 commits into
MonoGame:develop3dfrom
shilrobot:sampler-state-fix
Oct 26, 2012
Merged

Fix for issue #847, "SamplerStates aren't properly applied when active texture changes"#889
tomspilman merged 2 commits into
MonoGame:develop3dfrom
shilrobot:sampler-state-fix

Conversation

@shilrobot

Copy link
Copy Markdown
Contributor

What we would consider sampler state in D3D is stored per-texture in GL, so you have to have the correct active texture unit & texture bindings active before you call glTexParameter within SamplerState.Activate().

This change stores the "last SamplerState used" on the Texture instance, to avoid redundant glTexParameter calls.

It's possible this might make some redundant glActiveTexture parameters in cases where you use a single texture with frequently changing sampler states, maybe there is a better way to design this... it would be nice if it was somehow integrated with the related loop in TextureCollection.SetTextures().

@shilrobot

Copy link
Copy Markdown
Contributor Author

Oops, meant to type issue #842 in title. I think this also fixes issue #847 though.

@elisee

elisee commented Oct 23, 2012

Copy link
Copy Markdown
Contributor

As a workaround, I had temporarily made the dirty checks always pass in my local working copy of MonoGame, which made the sample states be correctly updated but was very slow since it meant the state were reset every time.

I just merged your commit in my own repository and I can confirm it seems to work properly (and gives much better performance that my workaround). So +1 for pulling this and thanks!

@tomspilman

Copy link
Copy Markdown
Member

We'll test this today and report back.

@shilrobot

Copy link
Copy Markdown
Contributor Author

OK, cool.

FYI, as I mentioned earlier, while this change is functional it is likely there is a more elegant way to do it. It is efficient compared to resetting the state every time, but it causes up to N extra glActiveTexture() calls for each draw where N SamplerState slots have changed, or N textures have changed. If the work could be integrated with TextureCollection.SetTextures(), we could avoid that. It also always loops over every sampler state slot (which can be like ~30) for each draw.

I have not attempted to make a change that addresses these issues myself, it seems like it would be more involved. Basically TextureCollection & SamplerStateCollection would need to share knowledge when rendering with GL. I think it is possible though.

@RayBatts

Copy link
Copy Markdown

Looks straight on iOS.

@tomspilman

Copy link
Copy Markdown
Member

If Steve agrees lets get this merged.

@KonajuGames

Copy link
Copy Markdown
Contributor

Tom, what are your thoughts on Scottish comments about TextureCollection
and SamplerStateCollection sharing info?

@KonajuGames

Copy link
Copy Markdown
Contributor

Damned auto correct. Scott's comments I meant.

@tomspilman

Copy link
Copy Markdown
Member

I agree... they should share info or maybe the application of texture/sampler state should occur in a single central location like GraphicsDevice. Still since this works right now and the other changes don't exist yet... we should probably merge the initial fix first.

@KonajuGames

Copy link
Copy Markdown
Contributor

Ok. I'm cool with that.

@dellis1972

Copy link
Copy Markdown
Contributor

This needs to be updated before we can merge

@shilrobot

Copy link
Copy Markdown
Contributor Author

Merged the latest from this repo's develop3d into my branch, hope that does it.

@shilrobot shilrobot mentioned this pull request Oct 26, 2012
@tomspilman

Copy link
Copy Markdown
Member

Yep... fixed.

Merging.

tomspilman added a commit that referenced this pull request Oct 26, 2012
Fix for issue #847, "SamplerStates aren't properly applied when active texture changes"
@tomspilman tomspilman merged commit 0ee113a into MonoGame:develop3d Oct 26, 2012
alxwest pushed a commit to alxwest/MonoGame that referenced this pull request May 3, 2024
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

Successfully merging this pull request may close these issues.

6 participants