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

general syntax fixes #3319

Merged
merged 90 commits into from
Sep 17, 2023
Merged

general syntax fixes #3319

merged 90 commits into from
Sep 17, 2023

Conversation

Brian-ED
Copy link
Sponsor Contributor

I don't know much C, but i found a spelling mistake and i tried making the code a bit prettier in raymath.c. I haven't tested everything, but it does compile and i don't think it needs testing since i didn't change any code, only spacing and comments.

Brian-E and others added 30 commits August 13, 2023 02:38
the commit renamed a bool to int and broke indentation: 233cf39
This commit is untested.
I don't know what consequences this has.
Since the commits that added these numbers were before epsilon was added,
I have assumed that epsilon could replace them.
…3241)

* Ignore unused return value of GetCodepointNext in GetCodepointCount

Removes the last warning from non-external libraries when compiling with
the default build configuration on x64 Linux.

* Remove unnecessary void cast in GetCodepointCount
This reverts commit e4dcbd5.
* Fix text_unicode.c example crashing

* Adjust the text_unicode.c example crashing fix
Since the key pressed are handle by comparing current vs previous
state (ie frame), a special way is needed to handle key repeats.
…e the image (#3264)

* Add a function to clone a sound and share data with another sound.

* rename items based on feedback

* PR Feedback, use custom unload for sound alias, not variant of normal sound unloading

* sound_multi example

* Validate that image rect drawing is inside the image so we don't overflow a buffer

* remove files that should not have been added.

* remove changes that should not have been

* revert

* adsfasdfsdfsdf
* [Feature] Add GetKeyRepeat

* Update rcore.c

* Simpler design, only one repeat per frame

* Update config.h

* Update rcore.c

* Add KEYBOARD_KEYS_MASK

* Update config.h

* reversions

* Update rcore.c

* Update rcore.c

* change docs

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update raylib.h

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c
Closes #3265.

The problem: LIBS_PRIVATE is a list of library names (used by pkg-config), but the shared library of the same name doesn't always exist.
@orcmid
Copy link
Contributor

orcmid commented Sep 16, 2023

I find omnibus pull requests to be excessive for a number of reasons, the first being the burden it imposes on reviewers. It also creates a kind of all-or-none situation that is undesirable for all of us.

I am also unclear whether some of these changes deal with some current issues that may be expected to change differently. I am a bit surprised there are no merge conflicts.

PS: In raymath.h there are too many stylistic changes, such as removing outer parentheses around a return-value expression. And adding a space in front of a "*" without one after it just seems wrong, because it looks like a pointer de-reference although it still parses as a multiplication. I suspect there are style preferences that Ramon has on these and that we should follow for consistency whether or not it is our preference in code of our own.

@ghost
Copy link

ghost commented Sep 16, 2023

I'm sorry, but is this PR really necessary? That's a lot of changes to review/test just for the sake of formatting. A mistake there could introduce new bugs/issues.

src/raymath.h Outdated Show resolved Hide resolved
src/raymath.h Outdated
view.m8*projection.m3 + view.m9*projection.m7 + view.m10*projection.m11 + view.m11*projection.m15,
view.m12*projection.m0 + view.m13*projection.m4 + view.m14*projection.m8 + view.m15*projection.m12,
view.m12*projection.m1 + view.m13*projection.m5 + view.m14*projection.m9 + view.m15*projection.m13,
view.m0 *projection.m0 + view.m1 *projection.m4 + view.m2 *projection.m8 + view.m3*projection.m12,
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to avoid this kind of alignments based on additional spaces. One space is ok despite lines are not aligned.

src/raymath.h Outdated
float a00 = matViewProj.m0, a01 = matViewProj.m1, a02 = matViewProj.m2, a03 = matViewProj.m3;
float a10 = matViewProj.m4, a11 = matViewProj.m5, a12 = matViewProj.m6, a13 = matViewProj.m7;
float a20 = matViewProj.m8, a21 = matViewProj.m9, a22 = matViewProj.m10, a23 = matViewProj.m11;
float a00 = matViewProj.m0 , a01 = matViewProj.m1 , a02 = matViewProj.m2 , a03 = matViewProj.m3;
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to avoid this kind of alignments based on additional spaces. One space is ok despite lines are not aligned.

src/raymath.h Outdated
@@ -990,22 +990,22 @@ RMAPI Vector3 Vector3Unproject(Vector3 source, Matrix projection, Matrix view)
float invDet = 1.0f/(b00*b11 - b01*b10 + b02*b09 + b03*b08 - b04*b07 + b05*b06);

Matrix matViewProjInv = {
(a11*b11 - a12*b10 + a13*b09)*invDet,
( a11*b11 - a12*b10 + a13*b09)*invDet,
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to avoid this kind of alignments based on additional spaces. One space is ok despite lines are not aligned.

src/raymath.h Outdated
float a00 = mat.m0, a01 = mat.m1, a02 = mat.m2, a03 = mat.m3;
float a10 = mat.m4, a11 = mat.m5, a12 = mat.m6, a13 = mat.m7;
float a20 = mat.m8, a21 = mat.m9, a22 = mat.m10, a23 = mat.m11;
float a00 = mat.m0 , a01 = mat.m1 , a02 = mat.m2 , a03 = mat.m3;
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to avoid this kind of alignments based on additional spaces. One space is ok despite lines are not aligned.

src/raymath.h Outdated
result.m7 = mat.m13;
result.m8 = mat.m2;
result.m9 = mat.m6;
result.m0 = mat.m0;
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to avoid this kind of alignments based on additional spaces. One space is ok despite lines are not aligned.

@raysan5
Copy link
Owner

raysan5 commented Sep 16, 2023

@Brian-ED Thanks for the fixes, I added some reviews but basically all are the same, I prefer to avoid column-aligned lines based on additional spaces. That convention is not used in raylib, only one space when required. Please, could you review it?

@Peter0x44
Copy link
Contributor

I think EPSILON is deliberately not used, so that you can copy any of the functions without needing any external things (it's why none of the functions in raymath call each other)

@Brian-ED
Copy link
Sponsor Contributor Author

I personally thought the whitespace helped reading a lot, but i will revert those

@raysan5
Copy link
Owner

raysan5 commented Sep 17, 2023

I personally thought the whitespace helped reading a lot, but i will revert those

It does but I prefer to stick to the formatting used all around raylib.

@Brian-ED
Copy link
Sponsor Contributor Author

Brian-ED commented Sep 17, 2023

I think EPSILON is deliberately not used, so that you can copy any of the functions without needing any external things (it's why none of the functions in raymath call each other)

Why does it exist then?
It's used in QuaternionEquals aswell, and other functions.

@raysan5
Copy link
Owner

raysan5 commented Sep 17, 2023

@Brian-ED I didn't noticed EPSILON existed somewhere, probably a user contribution. In any case, it can be defined inside the functions requiring it as:

#if !defined(RAYMATH_EPSILON)
    #define RAYMATH_EPSILON 0.0001
#endif

So the functions are portable but still external users can redefine it if required

@Brian-ED
Copy link
Sponsor Contributor Author

#if !defined(RAYMATH_EPSILON)
    #define RAYMATH_EPSILON 0.0001
#endif

The current name for epsilon is just EPSILON and not RAYMATH_EPSILON. Do you want me to replace EPSILON with RAYMATH_EPSILON? I think it will break backwards compatibility

@raysan5
Copy link
Owner

raysan5 commented Sep 17, 2023

@Brian-ED oh, I see it's already defined at the top, yeah, then better keep it as EPSILON, I would review it in the future if needed

@Brian-ED
Copy link
Sponsor Contributor Author

I am done with this pull, any extra comments are welcome.

I will make a new pull request for adding the #if !defined EPSILON ... actually, so i don't need to revert the epsilon changes in this pull.

@raysan5 raysan5 merged commit acf211a into raysan5:master Sep 17, 2023
12 checks passed
@raysan5
Copy link
Owner

raysan5 commented Sep 17, 2023

@Brian-ED thanks for the review.

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