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

Adds Controls Config file as a JSON #7

Closed
wants to merge 7 commits into from
Closed

Conversation

Viv-0
Copy link

@Viv-0 Viv-0 commented Jan 30, 2024

Adds a controls config file as a JSON.
Tested against Windows 10 with Controller and Keyboard
Supports deadzones for joysticks/triggers
Please send feedback if you have concerns

Copy link

@steviegt6 steviegt6 left a comment

Choose a reason for hiding this comment

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

Also lots of indentation inconsistencies.

Comment on lines +186 to +192
#pragma warning disable CS8600
JsonNode doc = JsonNode.Parse(File.ReadAllText(controlsPath));
if (doc != null)
Controls.HandleCustomControls(doc);
}

// wait for tasks to finish
{
foreach (var task in tasks)
#pragma warning restore CS8600

Choose a reason for hiding this comment

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

Why pragma instead of !?

Comment on lines +130 to +131
#pragma warning disable CS8604

Choose a reason for hiding this comment

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

Why not just handle nullability?

#pragma warning disable CS8604
// i probably should have handled it a bit better but :(, im just hoping that this isn't the slowest code in the game. - Viv
public static void HandleCustomControls(JsonNode root)

Choose a reason for hiding this comment

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

Would it not be better to deserialize to a type-checked object?

Choose a reason for hiding this comment

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

This existing code is extremely volatile and will cause problems if even minor issues are present.

@NoelFB
Copy link
Contributor

NoelFB commented Jan 30, 2024

Hey thanks for writing this all out! This is a great start, but I also agree with steviegt6 on a few issues:

  • Using null checks/handling null is better than using pragma's to ignore them. Ideally (aside from external libs) we wouldn't ignore any warnings.
  • I think creating a typed class to automatically serialize into might be worth doing. It might also make parsing the end data a lot easier. Since we're trimming the exe it will need a JsonSerializerContext but that's fairly trivial to do (see how Save.cs does it)

@bitten2up
Copy link

just tested this out, seems like the pause menu isn't working at least for keyboard

a quick look through the changes I'm not quite sure what the issue is

I have tried both a debug build and a release build on linux and both can't open it

@Viv-0
Copy link
Author

Viv-0 commented Jan 30, 2024

Regarding this, I think in order to use a Json-deserializing structure it would require rewriting things in terms of solely buttons, similar to the structure within Celeste itself, i.e.:
"MoveUp": { "Controller": ["LeftStickUp", "DPadUp"], "Keyboard": ["Up"] }

I'm going to rewrite it terms of that but I'm not sure whether or not I should then include Deadzones for things like Joystick/Triggers
edit: I'm going to add it as a "ControllerDetail" property and rewrite the structure a bit.

@steviegt6
Copy link

You can write your own (de)serializers.

SnipUndercover pushed a commit to SnipUndercover/Celeste64 that referenced this pull request Feb 4, 2024
feat: Add guide for Linux and MacOS users
@NoelFB
Copy link
Contributor

NoelFB commented Feb 4, 2024

I ended up doing an implementation getting it working, based on the ideas here! Appreciate the first pass that was done, thank you!

It's still missing Mouse to aim, but I think that could be added as a separate PR (and probably part of Foster somehow).

@NoelFB NoelFB closed this Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants