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

New Input Enhancements #155

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

literary-programmer
Copy link
Contributor

This PR adds new functions and their tests.

  1. bool has_keyboard(): this function tells you if there is no keyboard atached to hte system. USeful when you want ot inform the user to attach the keyboard.
  2. string get_key_name();: this function returns the key name for the scan code, aka KEY_* constants.

THis is the initial PR. I have a plan to add more functions, their tests, and possibly docs.

THis commit adds 2 new functions to input.cpp

1. string get_key_name();
2. bool has_keyboard();
This commit also fixes audio_form
New function `bool set_key_name(ke_code key, string name)`
This function will alter the key name returned by get_key_name.
@samtupy
Copy link
Owner

samtupy commented Jan 19, 2025

Hi,

Thanks for the changes thus far.

Unfortunately, a backwards compatibility problem presents us from easily switching the integer in the functions that take keycodes to a key_code enumeration. This is because somebody might have code like this:

int number = KEY_1 + i;
bool pressed = key_pressed(number);

Which would not compile should this new pr be merged without an extra cast being added to users code.

It seems like you already ran into this yourself, needing to update form.nvgt at least once to match this. If you had to update form.nvgt, imagine how much code people would have to change just for this?

For example in keyhold.nvgt, an include many old games are still using (even though it's not recommended), some keycodes are stored in integer variables. All such games would stop working unles they receive updates if we were to make this change.

In fact, any function such as key_code[]@ keys_pressed() for example I was thinking of changing back to integer.

If there was a significant consequence to these functions taking an integer rather than a key_code, I would consider that we should update to an enum. However, the benifits of switching to an enum over an integer are quite neglajable, the type safety added here is indeed minimal. For example I could still execute bool pressed = key_pressed(key_code(2222222)), which might result in the function being called with a value that isn't in the enumeration anyway. As such, it seems best to leave this bit lone, or, at least, to provide overloads for the functions in bgt_compat.nvgt that take integers, such as:

bool key_pressed(int key) { return key_pressed(key_code(key)); }

But we should consider carefully whether this would cause frustration for users when upgrading regardless.

@ethindp
Copy link
Collaborator

ethindp commented Jan 19, 2025

It may be worth adding engine overloads and adding a deprecation warning in compiler messages, perhaps?

@literary-programmer
Copy link
Contributor Author

We need to think of a solution before it gets too late. Like, version 1 hasn't released, and we are in the beta faze.

Either of the suggestions could work.

  1. We can add function overloads in the engine and show compiler messages, like @ethindp has suggested.
  2. We can make functions in the BGT compatibility layer.

For implementing either of the solutions, I have some questions.

  1. What will happen if multiple signatures are added in the compatibility script or engine and a number is passed to the functions?
  2. How do you add compiler message to the engine?
  3. Other than form.nvgt, I got to know that @ivansoto0 has used integers in his touch.nvgt, and if we go without making either of the change, we have to modify touch.nvgt, too.
  4. Other than that, we bgt_compat.nvgt, the key constants have been defined as uint. SO, either we have to make all of these constant types to key_code or bring the over loading functions. The question goes back to the start, where to place over loading functions?
  5. If we drop the idea of changing all that, how far in the future will be able to make this change? Won't it haunt us later?

@ethindp
Copy link
Collaborator

ethindp commented Jan 19, 2025

I think your overreacting here. There is no version 1 release deadline. Version 1 will happen when it happens, and we have a ton to figure out before that can even happen to begin with.

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.

3 participants