implement Rotate={0,1,2,3} in the GLES codepath

littleguy77 commented 9 years ago

~~This would be useful for working around the Adreno driver bug in the Android edition. Tagging @paulscode~~

@richard42 Would this necessitate a bump to the config version? I was considering submitting a pull request to add a config option for polygon offset values, which would also go in the Video-General section if accepted.

krnlyng commented 9 years ago

@littleguy77 looking forward to your pull request, will you do the same for glide64mk2? (will it fix the wobbly/moving ground issue? https://github.com/mupen64plus/mupen64plus-video-glide64mk2/issues/21) btw. i've implemented the same in glide64mk2, see https://github.com/mupen64plus/mupen64plus-video-glide64mk2/pull/20

littleguy77 commented 9 years ago

@krnlyng Adding a polygon offset setting to video-general would enable mobile developers to workaround the mobile hardware issues without modifying the mupen64plus source. Specifying non-default values in the config file would eliminate at least some of the issues you describe in https://github.com/mupen64plus/mupen64plus-video-glide64mk2/issues/21.

Ah, I just reailized this pull request was for Rice. My Adreno comment refers to glide. I'll post on the other pull request.

littleguy77 commented 9 years ago

We have had reports on Android of numerous regressions in video-rice and I tracked it down to this commit. Most of the reports describe "garbage" artifacts. A more benign example is this one, where the sparkles are rendered outside the letter-boxed area at the top and bottom (I have personally verified this): super_mario_64-006

Issues caused by this commit have also been discussed before in issues #24 and #25.

Given that

@Narann has a big refactoring/rewrite pending, and
@Gillou68310 describes at least a partial fix at https://github.com/Gillou68310/mupen64plus-video-rice/commit/c3c17c82c424b89f60e8cd18ada62a82681d5433

I wonder what is the best way to proceed? Is this fixable? Is the only real solution to revert? :( Should we just wait until @Narann's PR is ready?

Narann commented 9 years ago

Thanks for report @littleguy77. As I said in #25 the purposed way has some side effect. Remove it manually (for test) should not be so complicate actually. Do you think you could remove those few lines and test again? https://github.com/krnlyng/mupen64plus-video-rice/commit/e9ef968bd5961181099072b710e1c200f88106f6

Edit: Or, try to add the @Gillou68310 fix manually even if I'm not sure it fix everything.

littleguy77 commented 9 years ago

I git bisected to identify this commit. Then I reset to head revision and reverted this commit, tested, and posted to paulscode forum. You can see the branch in https://github.com/mupen64plus-ae.

My question is now that it's identified and confirmed, how do you want me to proceed... Submitting a PR that simply reverts may be too extreme.

Narann commented 9 years ago

I don't understand:

Then I reset to head revision and reverted this commit, tested, and posted to paulscode forum.

Ok, but as you don't provide a link to your post I can't guess if you fixed the problem or not.

So, can you confirm manually remove https://github.com/krnlyng/mupen64plus-video-rice/commit/e9ef968bd5961181099072b710e1c200f88106f6 fix the problem?

Why removing this PR seems too extreme? 1) The current way to rotate only affect one part of the color combiner (the 1/2 cycle). So other modes still don't rotate anything. 2) The provided fix actually blow the code to every stage of the color combiner to do the transform. I think it's not "the way it should be done".

So I removed this from my repo (and I forgot it was also in the main repo).

That's why I think removing this PR from the code is not such a bad idea, specially if it had such side effect.

So:

Could you confirm you fixed the problem removing https://github.com/krnlyng/mupen64plus-video-rice/commit/e9ef968bd5961181099072b710e1c200f88106f6.
If so, could you do a PR removing this PR? :)

littleguy77 commented 9 years ago

Maybe there's a language barrier? I tried to say twice now, I already did remove commit e9ef968 (using git revert) and confirmed myself that it does indeed fix the regressions.

Sorry, my previous link was broken (was typing on tablet). Here's the correct one. https://github.com/mupen64plus-ae/mupen64plus-ae/commit/914dbc3c650fa7d426931f810e8281f5a4543d2e

littleguy77 commented 9 years ago

I will submit the PR now to revert this commit.

Narann commented 9 years ago

Maybe there's a language barrier?

Haha maybe. I understood what you say imply the problem was fixed but as code is quite fragile I prefer explicit words. ^^

Merged.

mupen64plus / mupen64plus-video-rice

implement Rotate={0,1,2,3} in the GLES codepath #17