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: Skip vertices with index = -1 #1269

Closed
wants to merge 1 commit into from

Conversation

degasus
Copy link
Member

@degasus degasus commented Oct 12, 2014

This patches emits NaNs for all position indices with the index -1. This vertices should be skipped, but this would require a huge redesign of the vertex loader. So just emit NaNs which results in skipped primitives.
This may be wrong if the winding of strips matters, for quads, ... - but it's still much better than now.

@neobrain
Copy link
Member

"this would require a huge redesign of the vertex loader." Why?

@degasus
Copy link
Member Author

degasus commented Oct 13, 2014

@neobrain I'm not pretty sure what happens after a skipped vertex. Will this vertex just be skipped, or will a new strip start with the next vertex? With the first way, the vertex loader may just have to return the amount of skipped vertices, but the second one needs to inline the index generator for splitting strips :/

@neobrain
Copy link
Member

@degasus my understanding is that the vertex will simply be skipped, as if it had not been uploaded in the first place.

@neobrain
Copy link
Member

In any case, please don't speak about "this will require huge redesigns" (justifying the introduction of an ugly hack into our code base) when it apparently is not even clear how the feature is expected to work in the first place.

@degasus
Copy link
Member Author

degasus commented Oct 14, 2014

It's still a too big redesign for me. That's why I did create this PR, to fix my regression: https://code.google.com/p/dolphin-emu/issues/detail?id=7442

@Parlane
Copy link
Member

Parlane commented Oct 16, 2014

@dolphin-emu-bot rebuild

@comex
Copy link
Contributor

comex commented Oct 16, 2014

I made a hwtest which seems to confirm that the individual vertex is skipped (not the primitive, like this PR). I'll send a PR for that repo later, I guess.

@neobrain
Copy link
Member

@comex awesome, thank you for confirming that :)

@degasus
Copy link
Member Author

degasus commented Nov 1, 2014

So, shall we merge this PR? I know it isn't the accurate solution, but it's an easy way to fix the regression in a short-term until the vertex loader is done properly.

@degasus degasus force-pushed the vertexloaderfixes branch 4 times, most recently from f8567b2 to a88f12e Compare November 1, 2014 17:26
This patches emits NaNs for all position indices with the index -1. This vertices should be skipped, but this would require a huge redesign of the vertex loader. So just emit NaNs which results in skipped primitives.
This may be wrong if the winding of strips matters, for quads, ... - but it's still much better than now.
@degasus
Copy link
Member Author

degasus commented Nov 1, 2014

I've added a comment which marks that this is just a hack.

@neobrain
Copy link
Member

neobrain commented Nov 1, 2014

Leaning on the "let's implement it properly or not at all" side of things.

Also, @comex what happened to that hwtest?

@JMC47
Copy link
Contributor

JMC47 commented Nov 28, 2014

Pinging again; I'm interested in the HWTest too. I'd like this issue fixed for the games affected.

@degasus
Copy link
Member Author

degasus commented Nov 29, 2014

The hardware test is already done, but I have no idea if it was merged. The result was as expected that this vertices are skipped. So this code is wrong as already declared in my commit message, but it would be a hacky workaround. A good solution would require lots of vertex loader changes imo. I'm going to keep this issue in my mind, but until then, we could either merge this PR or let it as it is.

@degasus
Copy link
Member Author

degasus commented Dec 22, 2014

PR 1713 has implemented this correctly

@degasus degasus closed this Dec 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants