Ported support for Princess Peach: Showtime! from sudachi #53

lvella · 2024-03-24T22:15:44+01:00

lvella commented

2024-03-24 22:15:44 +01:00

Supposedly this newly implemented emitter is needed by "Princess Peach: Showtime!", but I am not sure at whet point, so I don't know how does it affects the game.

Anyway, this is a code path that was missing from our shader generators, so I'll leave this PR here...

Supposedly this newly implemented emitter is needed by "Princess Peach: Showtime!", but I am not sure at whet point, so I don't know how does it affects the game. Anyway, this is a code path that was missing from our shader generators, so I'll leave this PR here...

lvella added 1 commit 2024-03-24 22:15:45 +01:00

suyu-ci / Check REUSE Specification (pull_request) Failing after 12s Details

codespell / Check for spelling errors (pull_request) Successful in 11s Details

suyu verify / Verify Format (pull_request) Failing after 1m29s Details

suyu verify / test build (linux-fresh, clang) (pull_request) Has been skipped Details

suyu verify / test build (linux-fresh, linux) (pull_request) Has been skipped Details

suyu verify / test build (linux-mingw, windows) (pull_request) Has been skipped Details

suyu verify / android (pull_request) Has been skipped Details

25d0ee4817 Added support for Princess Peach: Showtime!

Exverge requested changes 2024-03-24 22:26:24 +01:00

Dismissed

Exverge left a comment

A copyright notice should be added to Sudachi.

~~I'll ask if we can implement this I couldn't get in contact as their website doesn't load and the dev doesn't have open DMs~~ my dms were closed

A copyright notice should be added to Sudachi. ~~I'll ask if we can implement this I couldn't get in contact as their website doesn't load and the dev doesn't have open DMs~~ my dms were closed

Exverge commented

2024-03-24 22:31:32 +01:00

@Crimson-Hawk

Crimson-Hawk force-pushed princess-peach-showtime from 25d0ee4817 to 0ae3868c0d

2024-03-25 00:57:32 +01:00

Compare

brujo55 commented

2024-03-25 15:27:09 +01:00

First-time contributor

i just tried the sudachi build with this fix and on android it doesn't solve anything

👍 1

Ryzendew commented

2024-03-25 17:54:11 +01:00

First-time contributor

It would be better to make our own fix instead. Also @lvella are you in the discord? I

Crimson-Hawk closed this pull request

2024-03-26 09:40:54 +01:00

lvella commented

2024-03-26 22:13:18 +01:00

Despite this not solving bug #21, I still think this PR is important, because it implements a code generation path that we are missing. Granted, almost no game uses geometry shaders (due to its terrible reputation), but in theory they can use.

A copyright notice should be added to Sudachi.

I think this would be unnecessary, because the attribution is properly given in the commit author. But it would also be incorrect, because Sudachi can not hold copyright (nor suyu, for that matter). Only legal entities can hold copyright, i.e. "Copyright 2024 Suyu" means nothing. Each change is copyrighted to the person or company who did it.

Despite this not solving bug #21, I still think this PR is important, because it implements a code generation path that we are missing. Granted, almost no game uses geometry shaders (due to its terrible reputation), but in theory they can use. > A copyright notice should be added to Sudachi. I think this would be unnecessary, because the attribution is properly given in the commit author. But it would also be incorrect, because Sudachi can not hold copyright (nor suyu, for that matter). Only legal entities can hold copyright, i.e. "Copyright 2024 Suyu" means nothing. Each change is copyrighted to the person or company who did it.

lvella commented

2024-03-27 01:13:47 +01:00

As requested in Discord, reopening.

lvella reopened this pull request

2024-03-27 01:13:47 +01:00

Exverge added the

bug

Needs Rebase

Needs Approvals

labels 2024-03-27 01:16:28 +01:00

lvella force-pushed princess-peach-showtime from 0ae3868c0d to 09578d522b

2024-03-27 01:47:38 +01:00

Compare

lvella added 1 commit 2024-03-27 01:51:09 +01:00

codespell / Check for spelling errors (pull_request) Successful in 12s Details

suyu-ci / Check REUSE Specification (pull_request) Successful in 14s Details

suyu verify / Verify Format (pull_request) Failing after 1m29s Details

suyu verify / test build (linux-fresh, clang) (pull_request) Has been skipped Details

suyu verify / test build (linux-fresh, linux) (pull_request) Has been skipped Details

suyu verify / android (pull_request) Has been skipped Details

73afddf1c2 Added support for Princess Peach: Showtime!

lvella force-pushed princess-peach-showtime from 73afddf1c2 to 077d7c8bfe

2024-03-27 02:12:56 +01:00

Compare

Exverge removed the

Needs Rebase

label 2024-03-27 02:22:09 +01:00

Exverge reviewed 2024-03-27 02:23:51 +01:00

Exverge left a comment

I (actually this time) asked the Sudachi author for permission to use this code, if he's okay with it I'll approve

lvella commented

2024-03-27 02:27:25 +01:00

I've added a new commit just to update the copyright notice on every file we got the changes from. Now the copyright holders are said to be the original "2021 yuzu Emulator Project", and the new "2024 sudachi Emulator Project".

As I stated in a previous comment, I think the entities actually holding the copyright are not properly represented, but I will not take the liberty to change what the authors themselves thought to write. What I did change was the year, because sudachi changes are obviously from 2024, not 2021 as originally stated in the sudachi files.

If this is not enough for you, feel free to submit a PR into my branch to explain how exactly do you want this attribution.

I've added a new commit just to update the copyright notice on every file we got the changes from. Now the copyright holders are said to be the original "2021 yuzu Emulator Project", and the new "2024 sudachi Emulator Project". As I stated in a previous comment, I think the entities actually holding the copyright are not properly represented, but I will not take the liberty to change what the authors themselves thought to write. What I did change was the year, because sudachi changes are obviously from 2024, not 2021 as originally stated in the sudachi files. If this is not enough for you, feel free to submit a PR into my branch to explain how exactly do you want this attribution.

👍 1

lvella force-pushed princess-peach-showtime from 077d7c8bfe to c6856ec53d

2024-03-27 02:27:33 +01:00

Compare

lvella commented

2024-03-27 02:30:04 +01:00

I (actually this time) asked the Sudachi author for permission to use this code, if he's okay with it I'll approve

I understand it is a courtesy, given the debacle that ended up in reddit, but as long as we don't misrepresent it, we already already had the permission from the moment he published his code as GPL.

> I (actually this time) asked the Sudachi author for permission to use this code, if he's okay with it I'll approve I understand it is a courtesy, given the debacle that ended up in reddit, but as long as we don't misrepresent it, we already already had the permission from the moment he published his code as GPL.

👍 1

lvella force-pushed princess-peach-showtime from c6856ec53d to 45df2ec6f9

2024-03-27 02:33:02 +01:00

Compare

lvella requested review from Exverge 2024-03-27 02:34:37 +01:00

Crimson-Hawk commented

2024-03-27 04:53:57 +01:00

shouldn't the copyright header be

// SPDX-FileCopyrightText: Copyright 2021 yuzu Emulator Project
// SPDX-FileCopyrightText: Copyright 2024 sudachi Emulator Project
// SPDX-FileCopyrightText: Copyright 2024 suyu Emulator Project

shouldn't the copyright header be ``` // SPDX-FileCopyrightText: Copyright 2021 yuzu Emulator Project // SPDX-FileCopyrightText: Copyright 2024 sudachi Emulator Project // SPDX-FileCopyrightText: Copyright 2024 suyu Emulator Project ```

Exverge approved these changes 2024-03-27 12:57:19 +01:00

Dismissed

Exverge left a comment

It was okayed by Jarrod. As long as you add @Crimson-Hawk’s changes, looks good

lvella commented

2024-03-27 16:33:38 +01:00

shouldn't the copyright header be

// SPDX-FileCopyrightText: Copyright 2021 yuzu Emulator Project
// SPDX-FileCopyrightText: Copyright 2024 sudachi Emulator Project
// SPDX-FileCopyrightText: Copyright 2024 suyu Emulator Project

I was not familiar with this SPDX standard, but for the little I could understand, you can only have one SPDX-FileCopyrightText: per file, so comma separating all the copyright holders within the same SPDX-FileCopyrightText: seems to be the correct way of doing it.

As I already said suyu Emulator Project is not a legal entity, thus it can't hold copyright (this wrongness applies to the two other notices, too). A more appropriate text would be suyu Project Developers;
But much more importantly, these files were not changed in any meaningful way by a suyu developer to warrant us copyright holdership.

> shouldn't the copyright header be > > ``` > // SPDX-FileCopyrightText: Copyright 2021 yuzu Emulator Project > // SPDX-FileCopyrightText: Copyright 2024 sudachi Emulator Project > // SPDX-FileCopyrightText: Copyright 2024 suyu Emulator Project > ``` I was not familiar with this SPDX standard, but for the little I could understand, you can only have one `SPDX-FileCopyrightText:` per file, so comma separating all the copyright holders within the same `SPDX-FileCopyrightText:` seems to be the correct way of doing it. About including `Copyright 2024 suyu Emulator Project`, I find it wrong in two ways: 1. As I already said `suyu Emulator Project` is not a legal entity, thus it can't hold copyright (this wrongness applies to the two other notices, too). A more appropriate text would be `suyu Project Developers`; 2. But much more importantly, these files were not changed in any meaningful way by a suyu developer to warrant us copyright holdership.

Crimson-Hawk approved these changes 2024-03-28 01:21:41 +01:00

Exverge removed the

Needs Approvals

label 2024-03-28 01:24:16 +01:00

Exverge requested changes 2024-03-28 01:27:14 +01:00

Dismissed

src/shader_recompiler/backend/glasm/emit_glasm_context_get_set.cpp Outdated

					
				@ -408,1 +408,4 @@

				        break;

				    case Stage::Geometry:

				        ctx.Add("SHL.U {}.x,{},16;", inst,

				                InputTopologyVertices::vertices(ctx.runtime_info.input_topology));

Exverge commented

2024-03-28 01:27:10 +01:00

/workspace/suyu/suyu/src/shader_recompiler/backend/glasm/emit_glasm_context_get_set.cpp:411:53: error: invalid use of incomplete type ‘const struct Shader::RuntimeInfo’

`/workspace/suyu/suyu/src/shader_recompiler/backend/glasm/emit_glasm_context_get_set.cpp:411:53: error: invalid use of incomplete type ‘const struct Shader::RuntimeInfo’`

Exverge commented

2024-03-28 01:28:08 +01:00

Looks like you're missing a runtime_info.h include

Looks like you're missing a `runtime_info.h` include

lvella commented

2024-03-28 12:02:22 +01:00

Sorry. The original patch included it in another header file. I was supposed to move it to emit_glasm_context_get_set.cpp, to have one less file to change the copyright notice, but I forgot. My local build is broken since d4375a21ef so I was relying on CI to build.

It is fixed now.

Sorry. The original patch included it in another header file. I was supposed to move it to `emit_glasm_context_get_set.cpp`, to have one less file to change the copyright notice, but I forgot. My local build is broken since d4375a21ef21 so I was relying on CI to build. It is fixed now.