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

VideoCommon: flush vertex manager if components change #3218

Merged
merged 1 commit into from Nov 2, 2015

Conversation

Tilka
Copy link
Member

@Tilka Tilka commented Nov 1, 2015

DON'T MERGE, this PR is only to get a FifoCI comparison. I replaced the workaround with a proper fix.

degasus helped a lot with explaining and debugging this issue.

@degasus
Copy link
Member

degasus commented Nov 1, 2015

LGTM

@Tilka Tilka changed the title VideoCommon: hacky workaround for issue 9005 VideoCommon: flush vertex manager if components change Nov 1, 2015
s_current_vtx_fmt = loader->m_native_vertex_format;
g_current_components = loader->m_native_components;

This comment was marked as off-topic.

@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • chibi-robo-zfighting on ogl-lin-intel: diff
  • ed-lighting on ogl-lin-intel: diff
  • melee-lighting on ogl-lin-intel: diff
  • milotic-texture on ogl-lin-intel: diff
  • sadx-ui on ogl-lin-intel: diff
  • soa-black on ogl-lin-intel: diff
  • ssbm-pointsize on ogl-lin-intel: diff
  • ztp-grass on ogl-lin-intel: diff
  • zww-armos on ogl-lin-intel: diff
  • zww-waves on ogl-lin-intel: diff
  • milotic-texture on ogl-lin-mesa: diff
  • milotic-texture on ogl-lin-nv: diff

automated-fifoci-reporter

@delroth
Copy link
Member

delroth commented Nov 1, 2015

Any reason to make shadergen depend on more global state vs. the previous way of passing the relevant state through args?

This doesn't seem to be justified and I see it as a regression.

@degasus
Copy link
Member

degasus commented Nov 2, 2015

@delroth This state could be passed by argument for SetShader(), but there is no (nice) way to do the same for VertexManager::Flush. Right now, only the vertex loader manager will know the last state, but almost every videocommon class might call VertexManager::Flush (which calls SetShader within the backends...). So we must still use a global. In my opinion, it is cleaner to fetch the global where it's needed (shadergen), not within a completely different subproject (videocommon vs backends). If we want to pass the "state" by argument, we should think about a united "GPU state"... But this is out of scope, so just use the same as all other states else sounds fine for me.

@delroth
Copy link
Member

delroth commented Nov 2, 2015

Agreed. Thanks for explaining why the change is necessary (VertexManager::Flush).

delroth added a commit that referenced this pull request Nov 2, 2015
VideoCommon: flush vertex manager if components change
@delroth delroth merged commit 89f6451 into dolphin-emu:master Nov 2, 2015
@Tilka Tilka deleted the components branch November 2, 2015 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants