-
Notifications
You must be signed in to change notification settings - Fork 430
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
Add source generator for HandleInputCache #5761
Conversation
|
||
public static bool RequestsPositionalInput(Drawable drawable) | ||
{ | ||
if (drawable is ISourceGeneratedInputCache sgInput && sgInput.KnownType == drawable.GetType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're gonna need to disable the suspicious type check inspections here because it doesn't look like R# can figure them out in the context of sourcegen...? Unless they're gonna go away after the sourcegen package is bumped I guess. (I wouldn't be surprised if they didn't, though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They'll go away with a package bump :)
osu.Framework.SourceGeneration/Generators/Input/HandleInputSemanticTarget.cs
Outdated
Show resolved
Hide resolved
osu.Framework.SourceGeneration/Generators/Input/HandleInputSemanticTarget.cs
Outdated
Show resolved
Hide resolved
I've re-read this - all code review suggestions look applied, source looks sane. That said, I'd probably want to spend a bit of time setting up local unit test runs for both framework and game with this sourcegen enabled, to see if there are any regressions, before I'd be 100% comfortable with leaving an approval. If you've already done this, or don't think that's necessary and that we can play it by ear whenever, then I can approve this and we can move forward. Otherwise, I'll set a few test runs up, but probably no sooner than this weekend. |
Reviewing this over again myself for the 5th time now... I believe there's still an issue if a non-partial class is the leaf node. ./sigh I think this is going to have to end up something like dependency injection after all. |
I'll hold off on the testing for now then until the code is a final state. |
Added back This PR is now once again ready to go... I hope. Just to be clear, this structure should be reviewed as a basis for all other such "simple" SGens. The |
So today I made a local sourcegen package, hooked it up to framework and a game checkout, ran full test suites on both (all passing), and confirmed the performance gains. I'm not sure it's time effective to be doing any further manual testing of this, so as far as I'm concerned this is probably as good to go as can get, and I'm gonna leave an approval. Not sure how we want to approach releasing this, however. What I'd probably do is:
But not sure my opinion holds any water here. @ppy/team-client thoughts? |
What we've done in the past is pushed an SGen nuget package from the PR, and updated the package ref in the PR. |
I'll go ahead with making the nuget package. @bdach fwiw you didn't approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops fixed
Prereqs: