raysan5 / raylib

A simple and easy-to-use library to enjoy videogames programming
http://www.raylib.com
zlib License
23.08k stars 2.3k forks source link

[core] Splitting `rcore` by platform into submodules #3313

Closed michaelfiber closed 1 year ago

michaelfiber commented 1 year ago

Issue description

As part of my experimenting in pr #3311 to divide rcore into submodules, one of the things I found was that the CoreData structure adds a lot of complexity to that endeavor and MAY benefit from being split up as well. I believe it may benefit because the current CoreData structure can be fairly difficult to understand with the large number of preprocessor directives within the struct itself. This is largely done to accommodate the many different target platforms. But if rcore is divided into different submodules based on platforms there is an opportunity to increase clarity.

Here are a couple of initial ideas for approaches:

  1. Maintain a single instance of CoreData but have CoreData only contain data that all submodules use so that it is a simpler struct with no preprocessor directives. A single CORE instance would be defined extern in rcore.h, defined in rcore.c, and accessed from rcore.c and all the submodules. In addition to this each submodule would have a small CoreData like struct of its own, i.e. DesktopData, WebData, etc. The data that is currently in CoreData that is specific to an submodule would be moved to the data struct in that submodule. Each struct definition would become much more legible because it wouldn't be full of preprocessor directives but there would be more structs overall and they'd be more spread out instead of centralized in rcore.

  2. Another option could be to have the CoreData struct defined entirely within a submodule specific header file so that each submodule has a complete CoreData struct and rcore.c can include a specific one based on the PLATFORM variable. There'd be more duplication but fewer structs and data would not be divided across multiple structs. Any changes to the data in CoreData that is used by all submodules would require applying the change to each submodule.

Personally I'm leaning towards the first option but am interested in what others think.

Environment

Experiments are being coded in Ubuntu 23.04 and leverage github actions to attempt to run build steps for all platforms.

Code Example

3311

raysan5 commented 1 year ago

on Android, I get these warnings for SetupFramebuffer(), SetupViewport(), InitTimer():

@Bigfoot71 yeah, I'm aware of those possible warning in some build systems, we can try to find a solution for them, for example declaring those functions also inside the platform modules... but those warning are probably more related to the build system than the compiler.

I merged these changes into my fork with DRM input updates and it all is working.

Really good to know! Feel free to send a PR with the new input system whenever you want!

Bigfoot71 commented 1 year ago

but those warning are probably more related to the build system than the compiler.

I see, that would indeed be more consistent. Finally, I don't think it's necessary to duplicate the declarations solely for this issue, which seems rather 'isolated' and doesn't pose any problems for the final product. My opinion.

Bigfoot71 commented 1 year ago

Following PR https://github.com/raysan5/raylib/pull/3420 by @ubkp, I also tried this on my end. Removing #include "rcore.h" seems to cause no issues when compiling raylib with its Makefile. However, it does cause these errors in Android Studio: log.txt

We might potentially find a macro definition that could help us determine whether to include this header or not. Alternatively, we could leave it by default, even if it's less consistent with the other headers.


A minor, somewhat peculiar thing to note is that the SetMousePosition() function is implemented on Android, which seems somewhat unnecessary unless there's a plan to implement a physical mouse on Android in the future.

Bigfoot71 commented 1 year ago

I would also like to know if there are any measures to be taken on Android if InitGraphicsDevice() (or even InitPlatform()) fails. Should we let the activity run with a black screen, or should we take steps to close it? (or any other action)

Because we don't do anything with their returns, not even returning them to the user.

hbiblia commented 1 year ago

This is somewhat silly, but wouldn't it be better to add the void OpenURL(const char *url) from rcore_desktop.c to rcore_template.c? The template has code that seems to be for Android.

image

raysan5 commented 1 year ago

This is somewhat silly, but wouldn't it be better to add the void OpenURL(const char *url) from rcore_desktop.c to rcore_template.c?

@hbiblia The platform split is still in progress, there is a lot of things to review, if you find something that can be improved, please, send a PR.

raysan5 commented 1 year ago

A minor, somewhat peculiar thing to note is that the SetMousePosition() function is implemented on Android, which seems somewhat unnecessary unless there's a plan to implement a physical mouse on Android in the future.

Yeap, it's there because those are all functions exposed by raylib.h and should be in the library, despite doing nothing.

I would also like to know if there are any measures to be taken on Android if InitGraphicsDevice() (or even InitPlatform()) fails. Should we let the activity run with a black screen, or should we take steps to close it? (or any other action)

In other platforms it just finishes the program, not sure if possible to finish the application on Android...

raysan5 commented 1 year ago

@michaelfiber @ubkp @Bigfoot71 Just added a new experimental platform: rcore_desktop_sdl.c. I implemented it in just a few hours and it works but not tested with all the examples and all the features. If anyone wants to help with it, you are welcome.

Please note that the building is up to the user at the moment, SDL2 include paths must be defines and program must be linked with SDL2.lib and SDL2main.lib.

michaelfiber commented 1 year ago

on Android, I get these warnings for SetupFramebuffer(), SetupViewport(), InitTimer():

@Bigfoot71 yeah, I'm aware of those possible warning in some build systems, we can try to find a solution for them, for example declaring those functions also inside the platform modules... but those warning are probably more related to the build system than the compiler.

I merged these changes into my fork with DRM input updates and it all is working.

Really good to know! Feel free to send a PR with the new input system whenever you want!

Yep, still testing but hopefully I'll have it ready soon. Gamepad support is looking good, mouse support is almost ready, then keyboard.

Bigfoot71 commented 1 year ago

In other platforms it just finishes the program, not sure if possible to finish the application on Android...

We can always deallocate resources and use ANativeActivity_finish(). Without going into details, the application's lifecycle might not necessarily be finished, and the library would remain "loaded", but the native activity itself would indeed be terminated.

To completely finish the application unconditionally, you could use exit(-1), but this does not respect the application's lifecycle and is therefore not recommended, perhaps only in the case of a critical error, but in our case it is just a black screen.

So either we finish the native activity, even though it doesn't necessarily terminate the entire application, or we leave the screen black, even though we don't handle any returns. (which is still strange, but hey)

ghost commented 1 year ago

@raysan5 This was an awesome surprise. Fantastic job!

I've managed to successfully compile PLATFORM_DESKTOP_SDL (84818c9) on Linux (Ubuntu 22.04 64-bit, gcc 11.2.0).

Tested quite a few examples and most things appear to be working. Just had issues with the input system:

  1. ~Mouse wheel getting stucked scrolling up or down;~ (#3428) :heavy_check_mark:
  2. ~Mouse sensitivity being really high on 3D;~ (#3428) :heavy_check_mark:
  3. ~Keypresses not working or working once;~ (#3435 by @Bigfoot71) :heavy_check_mark:
  4. ~Arrow keys causing segFaults;~ (#3435 by @Bigfoot71) :heavy_check_mark:
  5. ~Alt tabbing causing segFaults;~ (#3435 by @Bigfoot71) :heavy_check_mark:
  6. ~Mouse presses not working or working once.~ (#3428) :heavy_check_mark:

Edits: 1. updated the post after figuring out what I was doing wrong; 2. editing and added another issue; 3, 4, 5. updating.

Bigfoot71 commented 1 year ago

@raysan5 I managed to compile and run the example core_3d_camera_first_person.c _(with PLATFORM_DESKTOPSDL https://github.com/raysan5/raylib/commit/73363f829b709dddedf21a5e6b696f37e44bce19) on Linux Mint 21.2 64-bit (Ubuntu 22.04) using gcc 11.4.0. It runs successfully but doesn't recognize the keys and even produces a segmentation fault when I use the arrow keys. The issue seems to originate from this section in PollInputEvents() - LINE 605:

// Keyboard events
case SDL_KEYDOWN:
{
    CORE.Input.Keyboard.currentKeyState[event.key.keysym.sym] = 1;

    if (event.key.keysym.sym == SDLK_ESCAPE)
    {
        CORE.Window.shouldClose = true;
    }
} break;

This problem arises because the key values in SDL do not match those in Raylib. Here's what I obtained for the up arrow key: WARNING: SDLK_UP=1073741906 - KEY_UP=265.

It seems that a complete key mapping is needed to resolve this issue.

Edit: It might be more prudent to do it based on scancodes, in my opinion, to maintain consistency across different keyboards.

Bigfoot71 commented 1 year ago

Otherwise, I feel a bit foolish. The errors related to Android Studio were due to the presence of rcore_android.c in my CMakeLists.txt sources. The issues with undeclared functions and also errors when removing the inclusion of rcore.h in rcore_android.c are no longer present.

Please accept my apologies for my oversight.

@ubkp You can therefore remove the inclusion of rcore.h from rcore_android.c if you wish. If you have any other changes to make, don't hesitate. I'll read the commits and update the sources on my end if necessary.

ghost commented 1 year ago

@Bigfoot71 Do you want to fix the keypress issue on SDL? If yes, then I'll skip it and continue testing other functions.

raysan5 commented 1 year ago

@ubkp thank you very much for the reviews!!!

@Bigfoot71 I just did a quick test for the inputs, it requires some rework, probably some mapping from raylib defines to SDL keycodes.

Bigfoot71 commented 1 year ago

Do you want to fix the keypress issue on SDL? If yes, then I'll skip it and continue testing other functions.

Absolutely! I'll get right on it.

I just did a quick test for the inputs, it requires some rework, probably some mapping from raylib defines to SDL keycodes.

Yes, that's what I'm going to do. Using scancodes is fine for you?

raysan5 commented 1 year ago

Using scancodes is fine for you?

No, it should use keycodes, that's what the system maps internally for different keyboards.

Bigfoot71 commented 1 year ago

@raysan5 I don't understand what you mean. We can obtain the scancodes in SDL_PollEvent() using event.key.keysym.scancode.

I've created two versions for translating SDL codes:

Additionally, the version for translating scancodes will be much easier to maintain.

Honestly, I don't think I understood what you meant.

Edit: Furthermore, raylib with GLFW also seems to use scancodes _(when I press the Z key on my keyboard, it registers as KEYW). So, why would you want to use keycodes with SDL?

ghost commented 1 year ago

Edit: Furthermore, raylib with GLFW also seems to use scancodes _(when I press the Z key on my keyboard, it registers as KEYW). So, why would you want to use keycodes with SDL?

@Bigfoot71 I could be wrong, but I think that's exactly the problem of scancodes.

  1. Lets say I have a IsKeyPressed(KEY_Q) Quit(); somewhere on the program.
  2. With keycodes only the key labelled as Q will trigger that, regardless of its physical position on the keyboard layout.
  3. With scancodes that Q will be on the Q physical position of QWERTY and on the A physical position of AZERTY, which IMHO becomes a problem, once you start exposing rebinding to the user/interface.
  4. So, unless I'm getting this wrong, I believe keycodes provide the desired operation. Isn't this the case?
Bigfoot71 commented 1 year ago

@Bigfoot71 I could be wrong, but I think that's exactly the problem of scancodes.

@ubkp Yes, and precisely, I don't see this as a problem.

Currently, raylib with GLFW operates this way, and it's a good thing because the developer can code their game as if it were intended for a QWERTY keyboard (for example, using WASD for movement), and they won't have to do anything to support AZERTY keyboards (and, consequently, ZQSD for movement).

In summary, this is beneficial for gameplay and simplifies the developer's life. However, indeed, if the developer expects the player to explicitly press the letter "Q", they'll need to press letter "A".

But this "issue" is already present in raylib with GLFW, so at least we maintain the same behavior between SDL and GLFW.

I don't see a problem with using keycodes and thus requiring the developer to set up a different configuration for each keyboard in their code (or make an edit menu in the settings). However, in my opinion, this should also be made consistent with raylib's GLFW version.


Edit: Honestly, in my opinion, the best approach would be to follow SDL's lead and give the developer the choice for different scenarios, but that would require rewriting a lot of things.

raysan5 commented 1 year ago

@ubkp @Bigfoot71 I could be confused and I should review this more carefully but afaik, at this moment raylib processes keycodes already mapped by GLFW, instead of scancodes. Afaik, PLATFORM_DRM also consumes keycodes, mapped by the OS to evdev. In any case, raylib behaviour with SDL should be the same than with GLFW.

I could be wrong on my assumption, this info needs to be verified.

The point is, if a message on the screen says "Press Q to quit", in an ideal scenario it should work on all keyboards (maybe current implementation does not).

Bigfoot71 commented 1 year ago

@raysan5 I can assure you 200% that when I press "A" on my AZERTY keyboard, KEY_Q is "true" on PLATFORM_DESKTOP. However, I do acknowledge that from experience, this is not the case on PLATFORM_WEB, where my letter "A" will indeed be KEY_A.

Edit: In any case, I've written both versions for SDL. Let me know if I should submit the pull request now or wait (and which of the two versions).

Bigfoot71 commented 1 year ago

The problem might not come from GLFW itself or its usage?


I'll take X11 as an example since I'm on Linux.

To obtain keys from scancodes, GLFW uses the static int translateKey(int scancode) function.

This function uses the _glfw.x11.keycodes array, which is created in the static void createKeyTables(void) function. In this function, I see the following:

{ GLFW_KEY_Q, "AD01" },

I've verified that this is correct for an American keyboard according to /usr/share/X11/xkb/symbols/us. But according to my keyboard layout in /usr/share/X11/xkb/symbols/fr, the code AD01 corresponds to "A":

key <AD01> { [     a,              A                                       ] };

So when I press "A," GLFW gets AD01, which it interprets as GLFW_KEY_Q.

This is obtained in processEvent(XEvent *event), which is called by _glfwPollEventsX11(void), and it's sent as an int key later in our function static void KeyCallback(GLFWwindow *window, int key, int scancode, int action, int mods) in rcore_desktop.c.


Am I mistaken somewhere, or does this seem to be the issue?

ghost commented 1 year ago

@Bigfoot71 @raysan5

So, the main issue with scancodes is that the developer has to figure out the keyboard layout to display (e.g.: on the interface) the correct key label.

And with keycodes non-QWERTY keyboards get keys "scrambled" accordingly to it's layout, so it depends on the developer implementing some kind of rebind interface so they user can have some input coherence.

Both appear to have some big downsides. But scancodes looks like more forgiving, since, at least, it apparently won't break coherence for non-QWERTY, even tho the labels may not match there.

raysan5 commented 1 year ago

So, the main issue with scancodes is that the developer has to figure out the keyboard layout to display (e.g.: on the interface) the correct key label.

@ubkp I think so. As per my understanding, scancodes are the raw keyboard values generated by a key-press on the keyboard, independently of the label of the key. It's up to the system OS to map correctly those scancodes to the equivalent keycodes (labeled), following some kind of "standard" criteria.

And with keycodes non-QWERTY keyboards get keys "scrambled" accordingly to it's layout, so it depends on the developer implementing some kind of rebind interface so they user can have some input coherence.

It seems GLFW does internally that keycode mapping to standard US keyboard to provide the keyboard keys definition of the following list. If the mapping was done to another keyboard type (i.e. AZERTY) the same issue will happen with the defines.

More info:

ref GLFW doesn't care what layout you are using, if you look at the documentation, the physical keys are named after what they would be with a US layout. So that is the intended behavior.

So, if I'm right on my reasoning, one solution would be ignore GLFW defines and do the mapping from scancodes to keycodes manually in raylib BUT, my question is, how do we know the correct scancode to keycode mapping we should use? Are scancodes universal to all keyboards? How do we know if a keyboard is QWERTY or other?

Sorry, I'm a bit lost will all this issue at the moment...

@Bigfoot71 I think you can send the scancode implementation and we can test how it works with other keyboards/platforms. If everything works as expected, then we keep it; if not, we consider the other option.

Bigfoot71 commented 1 year ago

@raysan5 The scancode mapping for SDL should work exactly the same as GLFW does currently on PLATFORM_DESKTOP. I'll keep the other version for keycodes in reserve just in case. I'll submit the pull request within the hour.

raysan5 commented 1 year ago

@Bigfoot71 Merged! Thanks for the improvement! I think with this addition and @ubkp function review, most of the functionality should be already in place!

I'm considering moving InitWindow()/CloseWindow() to rcore.c and just keep InitPlatform()/ClosePlatform() implementations in rcore_platform.c, this way many duplicate code would be removed. Thinking about the best approach for that...

ghost commented 1 year ago

Splitting InitWindow()/CloseWindow() is an excellent idea!

Update: there's just a few functions missing: Function Status PR
ToggleFullscreen :heavy_check_mark: #3439 by @Bigfoot71
ToggleBorderlessWindowed :heavy_check_mark: #3439 by @Bigfoot71
SetWindowState :heavy_check_mark: #3439 by @Bigfoot71
ClearWindowState :heavy_check_mark: #3439 by @Bigfoot71
SetWindowIcons :heavy_check_mark: #3439 by @Bigfoot71
SetWindowMonitor :heavy_check_mark: #3439 by @Bigfoot71
GetMonitorPosition :heavy_check_mark: #3439 by @Bigfoot71
SetWindowFocused :heavy_check_mark: #3436
GetWindowHandle :heavy_check_mark: #3436
GetMonitorPhysicalWidth :heavy_check_mark: #3436
GetMonitorPhysicalHeight :heavy_check_mark: #3436
SetClipboardText :heavy_check_mark: #3436
GetClipboardText :heavy_check_mark: #3436
SetGamepadMappings :heavy_check_mark: #3436
ShowCursor :heavy_check_mark: #3436
HideCursor :heavy_check_mark: #3436
EnableCursor :heavy_check_mark: #3436
DisableCursor :heavy_check_mark: #3436
SetMouseCursor :heavy_check_mark: #3436

I'll try to finish that overnight. I could fall short of a few that involve a bit more to port tho. But, on those, I'll try to at least add a basic implementation, so it can be reviewed later with more time.

Edits: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10. updated list.

raysan5 commented 1 year ago

Splitting InitWindow()/CloseWindow() is an excellent idea!

Done! It seems everything still works as expected!

@ubkp Thank you very much for listing the missing functions and working on them! I'm sure SDL could cover most of them!

With those implemented, the rcore_desktop_sdl.c platform will be mostly complete! After merging it I'm considering moving the platform implementations to a platform directory for better organization.

michaelfiber commented 1 year ago

@ubkp @Bigfoot71 I could be confused and I should review this more carefully but afaik, at this moment raylib processes keycodes already mapped by GLFW, instead of scancodes. Afaik, PLATFORM_DRM also consumes keycodes, mapped by the OS to evdev. In any case, raylib behaviour with SDL should be the same than with GLFW.

I could be wrong on my assumption, this info needs to be verified.

The point is, if a message on the screen says "Press Q to quit", in an ideal scenario it should work on all keyboards (maybe current implementation does not).

In DRM there's a hard coded map in raylib mapping the scan codes to key codes. I've been looking into the most straight forward way to read the map file for the system and use that instead because then it'll match what the user sets.

ghost commented 1 year ago

Guys, did as many as I could overnight (#3436). Updated the table at https://github.com/raysan5/raylib/issues/3313#issuecomment-1767250636.

Already started work on ToggleFullscreen, ToggleBorderlessWindowed, SetWindowMonitor and GetMonitorPosition since they share a lot of logic. I should be able to deliver them during the day.

If anyone wants to take SetWindowState, ClearWindowState, SetWindowIcons, please, feel free to do so.

Bigfoot71 commented 1 year ago

@ubkp I started SetWindowState, ClearWindowState, SetWindowIcons, it seems that I already only have SetWindowIcons to do and then tested all that.

Bigfoot71 commented 1 year ago

There is a problem with DisableCursor(), the function hides the cursor for me but it is not brought back to the center of the window, which means that it is stuck at its edges. (SDL 2.0.20)

I know I had this problem a long time ago and fixed it but I no longer have the slightest idea how...


Another issue I've noticed (still regarding SDL) is that IsKeyPressed behaves like IsKeyDown and continuously returns true as long as the key is pressed.

This is strange because we have this before the switch statement:

    // Register previous keys states
    // NOTE: Android supports up to 260 keys
    for (int i = 0; i < 260; i++)
    {
        CORE.Input.Keyboard.previousKeyState[i] = CORE.Input.Keyboard.currentKeyState[i];
        CORE.Input.Keyboard.keyRepeatInFrame[i] = 0;
    }

If someone could check why that would be cool, I'll try it out once I'm done with implementing the functions mentioned in the previous message.

ghost commented 1 year ago

There is a problem with DisableCursor(), the function hides the cursor for me but it is not brought back to the center of the window, which means that it is stuck at its edges. (SDL 2.0.20)

I know I had this problem a long time ago and fixed it but I no longer have the slightest idea how...

@Bigfoot71 Yeah, it appears the GLFW and SDL diverge a bit on what they consider "locking the cursor".

When I went through DisableCursor initially I was having a hard time finding an equivalent on SDL, the GLFW_CURSOR_DISABLED name threw me off (L985). But checking the GLFW documentation it said _"GLFW_CURSORDISABLED hides and grabs the cursor, providing virtual and unlimited cursor movement. This is useful for implementing for example 3D camera controls."

The closest to that I could find was SDL_SetRelativeMouseMode which the SDL documentation mentions "While the mouse is in relative mode, the cursor is hidden, the mouse position is constrained to the window, and SDL will report continuous relative mouse motion even if the mouse is at the edge of the window."

But when I tested here (printing the cursor position, see example below), SDL_SetRelativeMouseMode still allowed the cursor to move freely (just hidden and limited to inside the window), so a single call like rcore_desktop.c (L988) didn't appear to work for SDL. I think that to keep the cursor trully centered it would have to be recentered at each frame or perhaps we have to normalize it's value (which is weird SDL_SetRelativeMouseMode doesn't already do).

#include "raylib.h"
#include <stdio.h>

int main(void) {
    InitWindow(800, 450, "test");
    SetTargetFPS(60);
    while (!WindowShouldClose()) {

        if (IsKeyDown(KEY_ONE))   { ShowCursor(); }
        if (IsKeyDown(KEY_TWO))   { HideCursor(); }
        if (IsKeyDown(KEY_THREE)) { EnableCursor(); }
        if (IsKeyDown(KEY_FOUR))  { DisableCursor(); }

        printf("%f %f \n", GetMousePosition().x, GetMousePosition().y);

        BeginDrawing();
        ClearBackground(RAYWHITE);
        DrawText("Test", 20, 20, 20, BLACK);
        DrawRectangle(20, 60, 40, 40, RED);
        EndDrawing();
    }
    CloseWindow();
    return 0;
}

@Bigfoot71 Would you like to handle this issue?

Bigfoot71 commented 1 year ago

@ubkp Yes, no problem I can take care of it, I should be able to find an old project where I was able to manage this issue.

Bigfoot71 commented 1 year ago

@ubkp Regarding the issue with DisableCursors(), it appears that there are two solutions.

First solution

The best solution would be to use SDL_SetHint(SDL_HINT_MOUSE_RELATIVE_WARP_MOTION, "1"); just after calling SDL_SetRelativeMouseMode(SDL_TRUE);, but it seems that my version of SDL (2.0.20) does not support it...

SDL 2.0.20 was released in January 2022 if I believe this link, and the feature SDL_HINT_MOUSE_RELATIVE_WARP_MOTION was added in August 2022 if I believe this other link.

Second solution

The other, slightly more cumbersome solution would be to call this function for each frame (when the cursor is disabled):

SDL_WarpMouseInWindow(platform.window, CORE.Window.display.width / 2, CORE.Window.display.height / 2);

Which will reposition the cursor to the center of the screen.

ghost commented 1 year ago

@Bigfoot71 I'm sorry, I missclicked and deleted the message.

EDIT: Trying to retype the previous message I deleted by accident. I'm so sorry.

@Bigfoot71 I tested SDL_SetHint(SDL_HINT_MOUSE_RELATIVE_WARP_MOTION, "1"); here on SDL 2.28.4 but it also didn't work for me.

But, while reviewing SDL_MouseMotionEvent (doc), I think we're polling the wrong field for relative mouse movement. Instead of (L713-L714):

CORE.Input.Mouse.currentPosition.x = (float)event.motion.x;
CORE.Input.Mouse.currentPosition.y = (float)event.motion.y;

We should be using:

CORE.Input.Mouse.currentPosition.x = (float)event.motion.xrel;
CORE.Input.Mouse.currentPosition.y = (float)event.motion.yrel;

We could add a cursorRelative to the PlatformData struct and use that to poll the correct fields on PollInputEvents. Something like:

if (cursorRelative)
{
CORE.Input.Mouse.currentPosition.x = (float)event.motion.xrel;
CORE.Input.Mouse.currentPosition.y = (float)event.motion.yrel;
}
else
{
CORE.Input.Mouse.currentPosition.x = (float)event.motion.x;
CORE.Input.Mouse.currentPosition.y = (float)event.motion.y;
}

It worked as expected on a quick test I did here.

@Bigfoot71 What do you think? If you agree, could you add that to #3439?

Bigfoot71 commented 1 year ago

@ubkp I saw it! Yes, event.motion.xrel and event.motion.yrel correspond to the delta, but that would indeed be quite clever! I think it would be a viable solution.

Edit: I'm trying this and if it's good I'll add it to the PR like you asked.

Edit 2: I spoke too quickly, it doesn't work and it's quite logical in the end :/

https://github.com/raysan5/raylib/assets/90587919/639d6083-f797-4b91-afd2-879b35fac365

ghost commented 1 year ago

@Bigfoot71 Awesome! Thank you very much! My apologies again for deleting the message. I was trying to do two things at once and messed up.

Bigfoot71 commented 1 year ago

Or we think that the problem comes from our way of obtaining the mouse delta. We could make sure that event.motion.xrel and event.motion.yrel are returned only when the mouse is moving.

Edit: This way the delta would always be correct and the mouse would not be stuck in the center of the screen when hidden. Because currently we calculate it ourselves with the position that we obtain ourselves, that's what poses the problem.

Edit 2: But it would be necessary to create a new variable to store the delta.

When trying SDL_WarpMouseInWindow as well, I notice the same behavior, which is logical since the mouse is repositioned each time. I think it would be really important to be able to store relx and rely to be able to return them in GetMouseDelta(), which is rather cumbersome.

Alternatively, we could see how SDL_SetHint(SDL_HINT_MOUSE_RELATIVE_WARP_MOTION, "1"); behaves, but there is a strong chance that it will yield the same result.

ghost commented 1 year ago

Edit 2: I spoke too quickly, it doesn't work and it's quite logical in the end :/

@Bigfoot71 Yeah, you're right. Also good idea using the core_3d_camera_first_person.

Or we think that the problem comes from our way of obtaining the mouse delta. We could make sure that event.motion.xrel and event.motion.yrel are returned only when the mouse is moving.

Edit: This way the delta would always be correct and the mouse would not be stuck in the center of the screen when hidden. Because currently we calculate it ourselves with the position that we obtain ourselves, that's what poses the problem.

This sounds like a good solution.

By the way, I was messing around testing zeroing the CORE.Input.Mouse.previousPosition (L615), which improved it, but caused drifting.

Edit: @Bigfoot71 Yeah, I'm trying to figure out why SDL_SetHint(SDL_HINT_MOUSE_RELATIVE_WARP_MOTION, "1"); is not working here. It should be working on 2.28.4.

ghost commented 1 year ago

@Bigfoot71 Managed to fix it. I'll clean this up and send you a diff. The solution was zeroing indeed.

ghost commented 1 year ago

@Bigfoot71 Could you please test this:

diff --git a/rcore_desktop_sdl.c b/rcore_desktop_sdl_FIXED.c
index cddee74..8657094 100644
--- a/rcore_desktop_sdl.c
+++ b/rcore_desktop_sdl_FIXED.c
@@ -63,6 +63,7 @@ typedef struct {

     SDL_Joystick *gamepad;
     SDL_Cursor *cursor;
+    bool cursorRelative;
 } PlatformData;

 //----------------------------------------------------------------------------------
@@ -529,7 +530,7 @@ void EnableCursor(void)
 {
     SDL_SetRelativeMouseMode(SDL_FALSE);
     SDL_ShowCursor(SDL_ENABLE);
-
+    platform.cursorRelative = false;
     CORE.Input.Mouse.cursorHidden = false;
 }

@@ -537,7 +538,7 @@ void EnableCursor(void)
 void DisableCursor(void)
 {
     SDL_SetRelativeMouseMode(SDL_TRUE);
-
+    platform.cursorRelative = true;
     CORE.Input.Mouse.cursorHidden = true;
 }

@@ -612,7 +613,8 @@ void PollInputEvents(void)
     CORE.Input.Mouse.currentWheelMove.y = 0;

     // Register previous mouse position
-    CORE.Input.Mouse.previousPosition = CORE.Input.Mouse.currentPosition;
+    if (platform.cursorRelative) CORE.Input.Mouse.currentPosition = (Vector2){ 0.0f, 0.0f };
+    else CORE.Input.Mouse.previousPosition = CORE.Input.Mouse.currentPosition;

     // Reset last gamepad button/axis registered state
     CORE.Input.Gamepad.lastButtonPressed = GAMEPAD_BUTTON_UNKNOWN;
@@ -710,8 +712,17 @@ void PollInputEvents(void)
             } break;
             case SDL_MOUSEMOTION:
             {
-                CORE.Input.Mouse.currentPosition.x = (float)event.motion.x;
-                CORE.Input.Mouse.currentPosition.y = (float)event.motion.y;
+                if (platform.cursorRelative)
+                {
+                    CORE.Input.Mouse.currentPosition.x = (float)event.motion.xrel;
+                    CORE.Input.Mouse.currentPosition.y = (float)event.motion.yrel;
+                    CORE.Input.Mouse.previousPosition = (Vector2){ 0.0f, 0.0f };
+                }
+                else
+                {
+                    CORE.Input.Mouse.currentPosition.x = (float)event.motion.x;
+                    CORE.Input.Mouse.currentPosition.y = (float)event.motion.y;
+                }
             } break;

             // Check gamepad events

What do you think? If you agree, feel free to add it to https://github.com/raysan5/raylib/pull/3439.

Bigfoot71 commented 1 year ago

@ubkp I try this right after!

Otherwise, do you think it is necessary to check SDL_WasInit() in certain functions related to the screen and the monitor? I probably would have done it but I see that the GLFW version does not check if the window has been initialized.

ghost commented 1 year ago

Otherwise, do you think it is necessary to check SDL_WasInit() in certain functions related to the screen and the monitor? I probably would have done it but I see that the GLFW version does not check if the window has been initialized.

@Bigfoot71 IMHO, I don't think it's necessary right now.

Bigfoot71 commented 1 year ago

@ubkp Your solution for the delta works! I'm adding it to the PR! (and we avoid storing an additional Vector2, even if it is less "explicit" it is rather clever!)

The only issue left is with IsKeyPressed, which always returns true when a key is pressed, just like IsKeyDown

https://github.com/raysan5/raylib/assets/90587919/d38f4a49-d664-476b-867d-e8d8b6c51e3b

ghost commented 1 year ago

The only issue left is with IsKeyPressed, which always returns true when a key is pressed, just like IsKeyDown

@Bigfoot71 Oh yeah, forgot about that. Let me take a look at it.

ghost commented 1 year ago

@Bigfoot71 I think I found the problem.

Looks like we are polling by event.type (L656) exclusively. But SDL_PRESSED and SDL_RELEASED are provided by event.state on the SDL_KeyboardEvent (doc).

I guess the solution could be checking if (event.key.state == SDL_PRESSED) inside case SDL_KEYDOWN: (L679). And checking if (event.key.state == SDL_RELEASED) inside case SDL_KEYUP: (L691).

What do you think?

Edit: code correction.

Edit 2, 3: ~tested it and event.state is not provided there. I'll take a deeper look into it.~ It is event.key.state. Working on a fix.

Bigfoot71 commented 1 year ago

@ubkp, look no further, I've found it, and it was quite simple. I had realized that it wasn't working for all the keys, and I immediately guessed that it was the loop for defining the previous keys.

Currently, we are doing this:

for (int i = 0; i < 260; i++)
{
    CORE.Input.Keyboard.previousKeyState[i] = CORE.Input.Keyboard.currentKeyState[i];
    CORE.Input.Keyboard.keyRepeatInFrame[i] = 0;
}

However, with MAX_KEYBOARD_KEYS set to 512, I did this, and it seems to work perfectly:

for (int i = 0; i < MAX_KEYBOARD_KEYS; i++)
{
    CORE.Input.Keyboard.previousKeyState[i] = CORE.Input.Keyboard.currentKeyState[i];
    CORE.Input.Keyboard.keyRepeatInFrame[i] = 0;
}
ghost commented 1 year ago

@Bigfoot71 Great catch! Tested here and that's indeed the fix. Great job!