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

VertexLoader: do not prepare for vertices if we need to skip them #739

Merged
merged 1 commit into from Aug 5, 2014

Conversation

delroth
Copy link
Member

@delroth delroth commented Aug 5, 2014

Fixes a recent regression from #716. Not a big fan of doing the DataSkip in VertexLoaderManager, but tbh it isn't much worse than doing it in VertexLoader. It also removes the dependency of VertexLoader on bpmem, which makes me happy.

@delroth
Copy link
Member Author

delroth commented Aug 5, 2014

Tested by @JMC47 to fix the regression, btw.

@@ -145,6 +146,13 @@ void RunVertices(int vtx_attr_group, int primitive, int count)
return;
VertexLoader* loader = RefreshLoader(vtx_attr_group);

if (bpmem.genMode.cullmode == 3 && primitive < 5)

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@lioncash
Copy link
Member

lioncash commented Aug 5, 2014

LGTM

lioncash added a commit that referenced this pull request Aug 5, 2014
VertexLoader: do not prepare for vertices if we need to skip them

Fixes issue [7542](https://code.google.com/p/dolphin-emu/issues/detail?id=7542)
@lioncash lioncash merged commit e170195 into dolphin-emu:master Aug 5, 2014
@degasus
Copy link
Member

degasus commented Aug 5, 2014

@lioncash I always fail here at the naming. Does "all" means all are passed or all are skipped? But in the end, the one case we filter here is the only uncommon one, so you'll notice such a issue very soon.

btw, as this case is quite uncommon, it's fine imo to move this skip where ever we want, eg VertexManager::Flush (which is already based on BP)

@degasus
Copy link
Member

degasus commented Aug 5, 2014

btw, I think the reason for this "hack" here is that opengl (maybe also d3d) doesn't support this culling mode (-> render nothing): http://www.opengl.org/wiki/Face_Culling

@lioncash
Copy link
Member

lioncash commented Aug 5, 2014

@degasus I don't know, I'm just going off of the enum here

@neobrain
Copy link
Member

neobrain commented Aug 5, 2014

@degasus "cull all" means all of the triangles are culled away (i.e. none of them are used for rendering). Indeed, this is required because OpenGL doesn't support it.

It might be useful for reference to know that this mode is used to set reference planes for zfreeze without actually rendering things on screen. Note that due to this it would be a severe problem for the software renderer if it had used the code including the workaround without further ado.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants