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.

Sign up for GitHub

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

Jump to bottom

Fix insane joystick allocations overhead #6115

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Fix insane joystick allocations overhead #6115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a71cd1c
Fix insane joystick allocations overhead
peppy Jan 9, 2024
6f8c571
Add note about everything being bad
peppy Jan 9, 2024
dc14cd7
Merge branch 'master' into fix-joystick-allocs
bdach Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions osu.Framework/Input/PassThroughInputManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ public void Sync(bool useCachedParentInputManager = false)
/// <param name="state">The state to synchronise current with. If this is null, it is regarded as an empty state.</param>
protected virtual void SyncInputState(InputState state)
{
// TODO: THIS IS ALL VERY HEAVY ALLOCATION AND NEEDS TO BE REMOVED.
// Using this class outside of a testing situation is not recommended...

// invariant: if mouse button is currently pressed, then it has been pressed in parent (but not the converse)
// therefore, mouse up events are always synced from parent
// mouse down events are not synced to prevent false clicks
Expand Down
12 changes: 9 additions & 3 deletions osu.Framework/Input/States/JoystickState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// See the LICENCE file in the repository root for full licence text.

using System.Collections.Generic;
using System.Linq;

namespace osu.Framework.Input.States
{
Expand All @@ -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 •
edited

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


/// <summary>
/// Retrieves all <see cref="JoystickAxis"/> with their current value (regardless of inactive ones).
/// </summary>
public IEnumerable<JoystickAxis> GetAxes() =>
AxesValues.Select((v, i) => new JoystickAxis((JoystickAxisSource)i, v));
public IEnumerable<JoystickAxis> GetAxes()
{
for (int i = 0; i < MAX_AXES; i++)
axes[i] = new JoystickAxis((JoystickAxisSource)i, AxesValues[i]);

return axes;
}
}
}