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

Remove "hardware" skinning #16345

Closed
wants to merge 6 commits into from
Closed

Remove "hardware" skinning #16345

wants to merge 6 commits into from

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Nov 6, 2022

After this, we only skin directly during decode. Much simpler, and is fast enough. "Hardware" (vertex shader skinning) didn't make good use of the hardware due to OpenGL limitations, and it's not really worth making a faster Vulkan implementation since skinning is never really a bottleneck these days.

Part of option cleanup - and as a benefit, lets us get rid of quite a large amount of code.

Will only reconsider if someone can show a popular device with a game that runs noticeably faster with hardware skinning than software skinning, which I think is unlikely.

@hrydgard hrydgard added the Code Cleanup Cleanup to make future work easier. Needs to be done sometimes. label Nov 6, 2022
@hrydgard hrydgard added this to the v1.14.0 milestone Nov 6, 2022
@ghost
Copy link

ghost commented Nov 6, 2022

This will affect bleach the heat soul 7 and monster hunter games in my mali 450 gpu #10775

@hrydgard
Copy link
Owner Author

hrydgard commented Nov 6, 2022

Oh right, we tried this once before, I forgot...

@hrydgard
Copy link
Owner Author

hrydgard commented Nov 6, 2022

Just weird to me that it makes such a difference in those games. Wonder if anything has changed....

@hrydgard hrydgard marked this pull request as draft November 6, 2022 16:24
@ghost
Copy link

ghost commented Nov 6, 2022

Just weird to me that it makes such a difference in those games. Wonder if anything has changed....

I will test again using the recently build on my mali 450 gpu phone.

@ghost
Copy link

ghost commented Nov 6, 2022

Only 5+ FPS is gain using hardware skinning in recently build 😅

HW
Screenshot_2022-11-07-00-34-20

SW
Screenshot_2022-11-07-00-32-59

@hrydgard
Copy link
Owner Author

hrydgard commented Nov 6, 2022

Well, still noticeable for sure. Although it'll only affect some games, and only one some old phones... Undecided, heh.

@ghost
Copy link

ghost commented Nov 6, 2022

Bleach The Heat Soul 7 on my vivo y51a adreno 306 is also benifit in HW Skinning.

HW Skinning
Screenshot_20221107_011125

SW Skinning
Screenshot_20221107_011053

@@ -823,15 +823,6 @@ static void check_variables(CoreParameter &coreParam)
g_Config.bHardwareTransform = true;
}

var.key = "ppsspp_software_skinning";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this doesn't actually remove the option in the other file I think?

-[Unknown]

// Changing the vertex type requires us to flush.
{ GE_CMD_VERTEXTYPE, FLAG_FLUSHBEFOREONCHANGE | FLAG_EXECUTEONCHANGE, 0, &GPUCommon::Execute_VertexType },
// Changing the vertex type requires us to flush, unless we just change the weight count - need to handle in a func.
{ GE_CMD_VERTEXTYPE, FLAG_EXECUTEONCHANGE, 0, &GPUCommon::Execute_VertexTypeSkinning },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We should probably just rename it.

-[Unknown]

GPU/Common/VertexDecoderCommon.h Outdated Show resolved Hide resolved
@unknownbrackets
Copy link
Collaborator

Hm, 25% is a lot. I feel like that's probably a case where it's using the same bones for a lot of (maybe offscreen?) drawing, which is the best case for hardware skinning I can imagine.

I wonder if there's a world in which we could decode using threads (for large draw calls, if that game even uses large ones) to close the gap?

I like the binding thing, though.

-[Unknown]

@@ -168,6 +168,13 @@ void SoftwareTransform::SetProjMatrix(float mtx[14], bool invertedX, bool invert
projMatrix_.translateAndScale(trans, scale);
}

static void ReadWeightedNormal(Vec3f &source, VertexReader &reader, u32 vertType) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can still have this func, but don't really need it now. If we do keep, we should probably use it for the initial read too.

-[Unknown]

@iota97
Copy link
Contributor

iota97 commented Nov 6, 2022

Just to note MH games probably show the most difference in the village area that have a lot of NPC with bones. That area is just a management hub so not really terrible if it lag a bit on old device.

AFAIR on my old phone HW was 30% slower than SW tho' (but many many build ago).

unknownbrackets added a commit that referenced this pull request Nov 21, 2022
Extract the Vulkan descriptor binding cleanup from #16345
@hrydgard hrydgard modified the milestones: v1.14.0, v1.15.0 Nov 23, 2022
@hrydgard hrydgard closed this Jan 10, 2023
@hrydgard hrydgard removed this from the v1.15.0 milestone Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Cleanup to make future work easier. Needs to be done sometimes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants