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

Ported xqemu nv2a changes #1457

Merged
merged 12 commits into from Oct 7, 2018

Conversation

Projects
None yet
6 participants
@PatrickvL
Member

PatrickvL commented Sep 28, 2018

This PR ports all xqemu nv2a changes up to xqemu/xqemu@3d33d81#diff-2042b2fa4b02c8eabddedcc278da5099

A big Thank You goes out to the people responsible for this original work - especially espes, Jannik Vogel (JayFoxRox) and Matt Borgerson, who've done a marvelous job!

This pull request was created by meticulously comparing all current nv2a code in xqemu, with the older copy we had in Cxbx-Reloaded. Everything new I've copied over, adding only the necessary changes to get the end result compiling in Cxbx-Reloaded.

By keeping everything in the same order, it's quite easy to do the same in the other direction : from Cxbx-Reloaded back to xqemu.
Note however, that there's only a few things present in the Cxbx-Reloaded version of the LLE NV2A emulation, that are not in xqemu:

  • The beginnings of a render plugin architecture, separating OpenGL code from the actual hardware emulation, so that other render API's can later be added (once this work is finished, it'd be great to see this ported back to xqemu)
  • Additions and updates (mainly sorting) to the NV2A register defines (this could be copied back to xqemu already)
  • Overlay drawing via an OpenGL fragment shader that converts YUYV to RGB on the GPU, avoiding the need for (CPU based) format conversion (since xqemu also emulates VGA, which Cxbx-Reloaded ignores, this is not easy to port back)

PS : Once finished, this fixes #1456

@literalmente-game

This comment has been minimized.

Show comment
Hide comment
@literalmente-game

literalmente-game Oct 1, 2018

Contributor

Tested with:

  • Forza
  • Colin McRae Rally 3
  • Evil Dead: A Fistful of Boomstick
  • Manhunt
  • Red Dead Revolver
  • The House of the Dead 3
  • Vietcong - Purple Haze

No regressions so far.

Contributor

literalmente-game commented Oct 1, 2018

Tested with:

  • Forza
  • Colin McRae Rally 3
  • Evil Dead: A Fistful of Boomstick
  • Manhunt
  • Red Dead Revolver
  • The House of the Dead 3
  • Vietcong - Purple Haze

No regressions so far.

@JayFoxRox

This comment has been minimized.

Show comment
Hide comment
@JayFoxRox

JayFoxRox Oct 1, 2018

I'm personally against the continous downstreaming, and instead propose upstreaming of your changes, and re-using XQEMU code as much as-is as possible (possibly as a submodule).
This might not be the best place to discuss this, but I already felt similar with #1437 and other recent changes.

The way Cxbx is currently using a major portion of the XQEMU code is probably not in the spirit of the GPL; open-source can't work this way. By now, it's basically a duplicate of the XQEMU source code being constantly downstreamed, with no changes making it upstream.
There are many cases where this is fine, but Cxbx and XQEMU goals are still mostly the same, so I feel this gives an unfair advantage to Cxbx-R as it's becoming essentially a hostile fork.

It frustrates me, that every change we do at XQEMU finds it way into Cxbx-R within a couple of hours / days, or even before it's merged into XQEMU (where the change was made for). It very much reminds me of all the Citra unofficial build drama (which I don't want for a hobby project).
I'm honestly considering to put a very restrictive license on my personal changes, and only GPL final versions for XQEMU use (at least until the contributions by both projects even-out).

This is not only true for the GPU portion, but also affects other parts of the Xbox ecosystem. I feel the past year or so, there has been less and less communication between the projects (despite more focus being put on XboxDev), but large chunks of the emulation relevant code has moved exclusively in one direction.
I'm unhappy with the current state, or at least how I perceive it.

Just to clarify: This is my personal opinion and might not reflect opinion of other XQEMU contributors.

JayFoxRox commented Oct 1, 2018

I'm personally against the continous downstreaming, and instead propose upstreaming of your changes, and re-using XQEMU code as much as-is as possible (possibly as a submodule).
This might not be the best place to discuss this, but I already felt similar with #1437 and other recent changes.

The way Cxbx is currently using a major portion of the XQEMU code is probably not in the spirit of the GPL; open-source can't work this way. By now, it's basically a duplicate of the XQEMU source code being constantly downstreamed, with no changes making it upstream.
There are many cases where this is fine, but Cxbx and XQEMU goals are still mostly the same, so I feel this gives an unfair advantage to Cxbx-R as it's becoming essentially a hostile fork.

It frustrates me, that every change we do at XQEMU finds it way into Cxbx-R within a couple of hours / days, or even before it's merged into XQEMU (where the change was made for). It very much reminds me of all the Citra unofficial build drama (which I don't want for a hobby project).
I'm honestly considering to put a very restrictive license on my personal changes, and only GPL final versions for XQEMU use (at least until the contributions by both projects even-out).

This is not only true for the GPU portion, but also affects other parts of the Xbox ecosystem. I feel the past year or so, there has been less and less communication between the projects (despite more focus being put on XboxDev), but large chunks of the emulation relevant code has moved exclusively in one direction.
I'm unhappy with the current state, or at least how I perceive it.

Just to clarify: This is my personal opinion and might not reflect opinion of other XQEMU contributors.

@LukeUsher

This comment has been minimized.

Show comment
Hide comment
@LukeUsher

LukeUsher Oct 1, 2018

Member

I do agree with JayFoxRox here. As it stands, the HLE parts of Cxbx-Reloaded and even most of the LLE portions (as small as they may be) are entirely our own work. This does not hold true for the nv2a lle, which is not much more than a copy from xqemu.

When I introduced the xqemu nv2a lle core, I had hoped it would lead to code sharing in both ways, with advances on both sides helping each other: more eyes on the code should have spurred improvements, but this has not happened, instead, the work seems to be developing in such a way that makes collaboration more difficult in the future. This is a situation that needs resolving.

The current situation is extremely unfair to the contributors to xqemu. I don’t want us to become, or appear to be, a project that leeches from the community without giving something back in return.

Multiple Xbox emulators can exist, but we must be careful in how we proceed. This is not how I saw the integration of the xqemu nv2a code ending up, and I fear that may have been a mistake.

Member

LukeUsher commented Oct 1, 2018

I do agree with JayFoxRox here. As it stands, the HLE parts of Cxbx-Reloaded and even most of the LLE portions (as small as they may be) are entirely our own work. This does not hold true for the nv2a lle, which is not much more than a copy from xqemu.

When I introduced the xqemu nv2a lle core, I had hoped it would lead to code sharing in both ways, with advances on both sides helping each other: more eyes on the code should have spurred improvements, but this has not happened, instead, the work seems to be developing in such a way that makes collaboration more difficult in the future. This is a situation that needs resolving.

The current situation is extremely unfair to the contributors to xqemu. I don’t want us to become, or appear to be, a project that leeches from the community without giving something back in return.

Multiple Xbox emulators can exist, but we must be careful in how we proceed. This is not how I saw the integration of the xqemu nv2a code ending up, and I fear that may have been a mistake.

@PatrickvL

This comment has been minimized.

Show comment
Hide comment
@PatrickvL

PatrickvL Oct 2, 2018

Member

There's no hostility here. Instead, the usage of the xqemu NV2A emulation code in Cxbx shows appreciation for the quality of that work, IMHO. Like you two, I agree it would be marvelous if this part could be submoduled. If anyone sees a way to accomplish that, please do! Note, there's nearly nothing unique to the Cxbx-copy of this code. From the top of my head, the only two things not in xqemu are the (incomplete) render plugin refactoring and hardware accelerated overlay rendering (by means of a fragment shader that converts YUYV to RGB).

Member

PatrickvL commented Oct 2, 2018

There's no hostility here. Instead, the usage of the xqemu NV2A emulation code in Cxbx shows appreciation for the quality of that work, IMHO. Like you two, I agree it would be marvelous if this part could be submoduled. If anyone sees a way to accomplish that, please do! Note, there's nearly nothing unique to the Cxbx-copy of this code. From the top of my head, the only two things not in xqemu are the (incomplete) render plugin refactoring and hardware accelerated overlay rendering (by means of a fragment shader that converts YUYV to RGB).

@AzurikRiseOfPerathia

This comment has been minimized.

Show comment
Hide comment
@AzurikRiseOfPerathia

AzurikRiseOfPerathia Oct 2, 2018

@JayFoxRox & @LukeUsher Let's create no competition, help us. Used each code, plugin from XQEMU to CXBX and vice versa. This will help advance both emulators in record time as each of them are on the side.

AzurikRiseOfPerathia commented Oct 2, 2018

@JayFoxRox & @LukeUsher Let's create no competition, help us. Used each code, plugin from XQEMU to CXBX and vice versa. This will help advance both emulators in record time as each of them are on the side.

@PatrickvL PatrickvL referenced this pull request Oct 4, 2018

Merged

Nv2a plugin clear #1461

@JayFoxRox

This comment has been minimized.

Show comment
Hide comment
@JayFoxRox

JayFoxRox Oct 7, 2018

@PatrickvL I know that there's no hostility intented (and none felt on my part), but this is still a serious issue in my opinion, which will eventually lead to an unintentional hostile fork of the XQEMU GPU.

I have just spent about 30 minutes marking some of the NV2A changes (of varying importance) to remind people to upstream on those PRs which had "NV2A" in the name and seemed relevant.

The issue: Cxbx-R has like 3 different people cleaning up and fixing bugs in the GPU, where XQEMU typically has at most 2-3 active people overall, who do a lot of housekeeping and ecosystem work (including critical GPU and APU research).
Cleanup PRs in Cxbx-R often affect Cxbx-R and XQEMU code, so there are PRs which touch 1000+ lines. It's next to impossible for us to know if there's anything of importance for us.
The formatting has also been changed, so we can't automate this comparison step either.

We therefore depend on communication and "upstream first" - but that's currently not happening.

So it's impossible for us to keep up with the changes, especially if the PRs only get exposure for a couple of hours - often Cxbx-R contributors merge their own PR, with little documentation what the code actually does.
So I manually have to work through the backlog, sometimes reading code re-formated by the IDE.

This leads to the Cxbx-R LLE GPU benefiting from XQEMU changes (which are typically public and in review for a long time, with many comments and hardware-tests), whereas the XQEMU GPU hardly ever benefits from changes in Cxbx-R (because we are not aware of them [even if we look for them] or sometimes don't reach expected code-quality [which we can't always tell, because no tests are provided]).

There are also other partial copies of XQEMU GPU in many projects already and we can't possibly monitor all forks for changes: There's only 1 upstream, but many downstreams.

If someone changes something, they should (try to) fix it upstream first.

Even if we reject changes for XQEMU upstream, it will be on record then (and those changes could still be merged in Cxbx-Reloaded then, to support more experimental behaviour over XQEMU).

One permanent solution to address this would be to give full ownership of the LLE GPU to XQEMU (and submodule XQEMU in Cxbx-R); which, I'll admit, has considerable drawbacks to Cxbx-R until XQEMU gains more maintainers, to keep up with the faster Cxbx-R workflow + the experimental aspect will be gone, as it will be harder to overlay changes for Cxbx-R.

JayFoxRox commented Oct 7, 2018

@PatrickvL I know that there's no hostility intented (and none felt on my part), but this is still a serious issue in my opinion, which will eventually lead to an unintentional hostile fork of the XQEMU GPU.

I have just spent about 30 minutes marking some of the NV2A changes (of varying importance) to remind people to upstream on those PRs which had "NV2A" in the name and seemed relevant.

The issue: Cxbx-R has like 3 different people cleaning up and fixing bugs in the GPU, where XQEMU typically has at most 2-3 active people overall, who do a lot of housekeeping and ecosystem work (including critical GPU and APU research).
Cleanup PRs in Cxbx-R often affect Cxbx-R and XQEMU code, so there are PRs which touch 1000+ lines. It's next to impossible for us to know if there's anything of importance for us.
The formatting has also been changed, so we can't automate this comparison step either.

We therefore depend on communication and "upstream first" - but that's currently not happening.

So it's impossible for us to keep up with the changes, especially if the PRs only get exposure for a couple of hours - often Cxbx-R contributors merge their own PR, with little documentation what the code actually does.
So I manually have to work through the backlog, sometimes reading code re-formated by the IDE.

This leads to the Cxbx-R LLE GPU benefiting from XQEMU changes (which are typically public and in review for a long time, with many comments and hardware-tests), whereas the XQEMU GPU hardly ever benefits from changes in Cxbx-R (because we are not aware of them [even if we look for them] or sometimes don't reach expected code-quality [which we can't always tell, because no tests are provided]).

There are also other partial copies of XQEMU GPU in many projects already and we can't possibly monitor all forks for changes: There's only 1 upstream, but many downstreams.

If someone changes something, they should (try to) fix it upstream first.

Even if we reject changes for XQEMU upstream, it will be on record then (and those changes could still be merged in Cxbx-Reloaded then, to support more experimental behaviour over XQEMU).

One permanent solution to address this would be to give full ownership of the LLE GPU to XQEMU (and submodule XQEMU in Cxbx-R); which, I'll admit, has considerable drawbacks to Cxbx-R until XQEMU gains more maintainers, to keep up with the faster Cxbx-R workflow + the experimental aspect will be gone, as it will be harder to overlay changes for Cxbx-R.

@RadWolfie

This comment has been minimized.

Show comment
Hide comment
@RadWolfie

RadWolfie Oct 7, 2018

Member

@JayFoxRox, to be clear. There are only 2 main developers for LLE GPU are @LukeUsher and @PatrickvL here. Plus we are still under manpower too.

Member

RadWolfie commented Oct 7, 2018

@JayFoxRox, to be clear. There are only 2 main developers for LLE GPU are @LukeUsher and @PatrickvL here. Plus we are still under manpower too.

@PatrickvL

This comment has been minimized.

Show comment
Hide comment
@PatrickvL

PatrickvL Oct 7, 2018

Member

@LukeUsher IF we are going to submodule a fork of xqemu to ease upstreaming our changes to the xqemu NV2A emulation code, then it's wise to merge this pull request, as it reduces the number of differences between our copy and xqemu's current NV2A emulation code

Member

PatrickvL commented Oct 7, 2018

@LukeUsher IF we are going to submodule a fork of xqemu to ease upstreaming our changes to the xqemu NV2A emulation code, then it's wise to merge this pull request, as it reduces the number of differences between our copy and xqemu's current NV2A emulation code

@LukeUsher

This comment has been minimized.

Show comment
Hide comment
@LukeUsher

LukeUsher Oct 7, 2018

Member

Okay I’ll merge this for now. I’m busy/away next week but after that, I’ll have a compare of our code vs xqemu and see what can be nicely upstreamed.

Member

LukeUsher commented Oct 7, 2018

Okay I’ll merge this for now. I’m busy/away next week but after that, I’ll have a compare of our code vs xqemu and see what can be nicely upstreamed.

@LukeUsher LukeUsher merged commit 81d52e5 into Cxbx-Reloaded:develop Oct 7, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@PatrickvL

This comment has been minimized.

Show comment
Hide comment
@PatrickvL

PatrickvL Oct 7, 2018

Member

See #1467 for possible future work on this

Member

PatrickvL commented Oct 7, 2018

See #1467 for possible future work on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment