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

[rcamera] Added missing CAMERA_CUSTOM check in UpdateCamera(Camera *camera, int mode) function #3938

Merged
merged 1 commit into from
Apr 28, 2024

Conversation

lima-limon-inc
Copy link
Contributor

@lima-limon-inc lima-limon-inc commented Apr 26, 2024

rcamera.h states that if UpdateCamera() is passed CAMERA_CUSTOM as an argument; the function will do nothing. Source: https://github.com/raysan5/raylib/blob/master/src/rcamera.h#L120

However, the implementation of UpdateCamera(), has no checks to see if the mode passed is CAMERA_CUSTOM.
https://github.com/raysan5/raylib/blob/master/src/rcamera.h#L438C6-L464

If UpdateCamera receives CAMERA_CUSTOM as argument; it will execute all the camera updating functions from lines 458 to 463.

This has the following effect:
https://github.com/raysan5/raylib/assets/65001595/a853549f-bb17-47af-b79b-ef2021d2ef12

After the patch is applied; this behavior changes:
https://github.com/raysan5/raylib/assets/65001595/121953c7-779c-4e41-9cbe-89f61abd9b00

Now UpdateCamera() won't modify the camera regardless of Mouse movement or keyboard movement (Which, is the intended behavior; right?)

This is the code that is used in the video:

Camera3D camera {
    {0, 0, 5},
    {0, 0, 0},
    {0, 1, 0},
    90,
    CAMERA_PERSPECTIVE,
};

CameraMode mode = CAMERA_FREE;

int main(void)
{
    // Initialization
    //--------------------------------------------------------------------------------------
    const int screenWidth = 800;
    const int screenHeight = 450;

    InitWindow(screenWidth, screenHeight, "raylib CAMERA_CUSTOM");

    SetTargetFPS(60);   // Set our game to run at 60 frames-per-second
    //--------------------------------------------------------------------------------------

    // Main game loop
    while (!WindowShouldClose())    // Detect window close button or ESC key
    {
        UpdateCamera(&camera, mode); 

        BeginDrawing();

	  BeginMode3D(camera);


	  ClearBackground(RAYWHITE);
	  if (IsKeyPressed(KEY_T)) {
	      printf("\n");
	      printf("I am teleporting the camera and setting it to custom mode\n");
	      printf("\n");
	      mode = CAMERA_CUSTOM;
	      camera.position.x = 0;
	      camera.position.y = 5;
	      camera.position.z = 0;
	  }
	  DrawText(TextFormat("Score: %08i", 999), 200, 80, 20, RED);

	  DrawCube({0,0,0}, 4, 4, 4, BLUE);

	  EndMode3D();
        EndDrawing();
    }

    // De-Initialization
    //--------------------------------------------------------------------------------------
    CloseWindow();        // Close window and OpenGL context
    //--------------------------------------------------------------------------------------

    return 0;

}

PS: I am not mistaken; this would be the only/first use of CAMERA_CUSTOM in raylib.

PS2: There's a difference in the comments in between raylib.h and rcamera.h. Is this intentional? Should I add another commit making them the same?
Rcamera (comment is slightly more verbose): https://github.com/raysan5/raylib/blob/master/src/rcamera.h#L120
Raylib (comment is more bare bones): https://github.com/raysan5/raylib/blob/master/src/raylib.h#L916

@justgo97
Copy link

You don't need to call UpdateCamera when using CAMERA_CUSTOM though I guess you can use UpdateCameraPro if you need it.

@lima-limon-inc
Copy link
Contributor Author

You don't need to call UpdateCamera when using CAMERA_CUSTOM though I guess you can use UpdateCameraPro if you need it.

Hi @justgo97 ! If I am not mistaken, the header file that I pointed to in the PR states that if UpdateCamera() is called with CAMERA_CUSTOM as an argument; the function "Will do nothing"

Wouldn't that mean that calling UpdateCamera with CAMERA_CUSTOM as an argument is an intended feature/something the function should handle?

  • Best regards

@justgo97
Copy link

You don't need to call UpdateCamera when using CAMERA_CUSTOM though I guess you can use UpdateCameraPro if you need it.

Hi @justgo97 ! If I am not mistaken, the header file that I pointed to in the PR states that if UpdateCamera() is called with CAMERA_CUSTOM as an argument; the function "Will do nothing"

Wouldn't that mean that calling UpdateCamera with CAMERA_CUSTOM as an argument is an intended feature/something the function should handle?

  • Best regards

Yes you are correct about the comment in the header not matching how the actual code works, Your PR would fix that.
Till this gets patched users need to do the check manually

    if (mode != CAMERA_CUSTOM) {
        UpdateCamera(&camera, mode); 
    }

@lima-limon-inc
Copy link
Contributor Author

You don't need to call UpdateCamera when using CAMERA_CUSTOM though I guess you can use UpdateCameraPro if you need it.

Hi @justgo97 ! If I am not mistaken, the header file that I pointed to in the PR states that if UpdateCamera() is called with CAMERA_CUSTOM as an argument; the function "Will do nothing"
Wouldn't that mean that calling UpdateCamera with CAMERA_CUSTOM as an argument is an intended feature/something the function should handle?

  • Best regards

Yes you are correct about the comment in the header not matching how the actual code works, Your PR would fix that. Till this gets patched users need to do the check manually

    if (mode != CAMERA_CUSTOM) {
        UpdateCamera(&camera, mode); 
    }

Oh; okey noted! I see what you mean!

As an aside; do yo have any comments regarding the patch? I know it isn't much! 😅

@justgo97
Copy link

Oh; okey noted! I see what you mean!

As an aside; do yo have any comments regarding the patch? I know it isn't much! 😅

I don't know much about the coding style conventions in this repo so probably a maintainer can comment on this.
I personally avoid empty braces and would do something like this:

void UpdateCamera(Camera *camera, int mode)
{
    if (mode == CAMERA_CUSTOM) {
        return;
    }

    // The rest of the function code
    
}

but again this is just a preference, it will still get the job done in it's current format.

@raysan5 raysan5 merged commit 10e702f into raysan5:master Apr 28, 2024
@raysan5
Copy link
Owner

raysan5 commented Apr 28, 2024

@lima-limon-inc Thanks for the patch! Personally I prefer that approach than an early return, I try to minimize/avoid early-returns in raylib as much as possible.

@lima-limon-inc
Copy link
Contributor Author

Hello @raysan5 ! Thank you very much!
I decided against using the early return in order to follow the style of the surrounding code.

As I side note, may I ask why you chose against them? Do they complicate compiler optimizations? Is it a style decision?

Side note 2: Raylib is a joy to use! Thank you so much for all of your work!

@raysan5
Copy link
Owner

raysan5 commented Apr 28, 2024

As I side note, may I ask why you chose against them? Do they complicate compiler optimizations? Is it a style decision?

It's just a style and organization decision, I prefer to know that all function return at the end (despite I use some early returns in some functions, mostly for security checks).

@lima-limon-inc
Copy link
Contributor Author

As I side note, may I ask why you chose against them? Do they complicate compiler optimizations? Is it a style decision?

It's just a style and organization decision, I prefer to know that all function return at the end (despite I use some early returns in some functions, mostly for security checks).

Great! Thanks for the insight. I made a follow up PR regarding the "PS2" I made. It only changes the comments in raylib.h so that they match the comments in rcamera.h

Here's the PR: #3942

Have a good day!

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