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

Fix insane joystick allocations overhead #6115

Merged
merged 3 commits into from Jan 9, 2024
Merged

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Jan 9, 2024

Before After
CleanShot 2024-01-09 at 09 55 33 CleanShot 2024-01-09 at 09 55 38

I'd love to remove that remaining enumerator but not sure on the best way. Open to advice:

/// <summary>
/// List of joystick axes.
/// </summary>
public readonly IEnumerable<JoystickAxis> Axes;

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 9, 2024

Turns out this only affects tests. And I'm not even going to try to fix the rest of the issues (basically PassThroughInputManager is broke beyond recovery).

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 9, 2024

Turns out it doesn't only affect tests. RulesetInputManager is one of these.

I'm going to have to fix the rest of this mess...

Will do that in separate PRs.

@bdach bdach self-requested a review January 9, 2024 16:21
@@ -22,10 +21,17 @@ public class JoystickState
/// </summary>
internal readonly JoystickButton[] AxisDirectionButtons = new JoystickButton[MAX_AXES];

private readonly JoystickAxis[] axes = new JoystickAxis[MAX_AXES];
Copy link
Collaborator

@bdach bdach Jan 9, 2024

Choose a reason for hiding this comment

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

I do wonder if this is worth the hassle. Like if GetAxes() is just a thin veneer over this array then you might as well expose it as public anyway and remove the method entirely. Heck all of the other input state classes do that.

Probably fine either way but I do wonder about your opinion.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I'm fine with that, but the main reason I didn't is because when I've done so before, issue has been brought with the non-immutability nature of arrays. Hinted on this in the OP (the fact we're casting this to IEnumerable also causes enumerator overheads).

Dunno, I lean more towards "we're making a game engine, not a financial framework which needs to guard against users doing silly things at all costs". We've sacrificed performance (and time to fix that performance gap) due to being overly careful over the years and I'm not sure how worth it that is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll go with this for now as not to slow this down over trivialities but other input state classes already do raw arrays so /shrug

@bdach bdach enabled auto-merge January 9, 2024 18:20
@bdach bdach merged commit b05c707 into ppy:master Jan 9, 2024
13 checks passed
@peppy peppy deleted the fix-joystick-allocs branch January 17, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants