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

Restructure and rephrase README #437

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

master30f
Copy link
Contributor

The current README.md contains a lot of redundant information which makes it hard to spot the actually important pieces. This pull request does the following:

  • restructures the file to remove unnecessary / redundant information
  • moves the long chapters about building into a separate file called BUILDING.md
  • rephrases the README to improve it's style and to fix gramatical errors

As the readme is the first thing anyone sees when finding this repo, I think it's important that it leaves a good impression and gets the most important information across as efficiently as possible.

Futher work could be done on BUILDING.md. As it is not as visible as the README, I've left it virtually unchanged, only rephrasing the titles.

@recp
Copy link
Owner

recp commented Jan 2, 2025

Maybe we should keep some piece of existing README?

  • "Highly optimized 2D|3D math library, also known as OpenGL Mathematics (glm) for C"
  • 🚀 Features
  • Some notes at "📌 Note for new comers (Important)" section?
  • Example: Computing MVP matrix ( to show multiplication order )

Also adding CGLM_OMIT_NS_FROM_STRUCT_API to README could help to discovery this feature: glms_mat4_mul() -> mat4_mul()

@master30f
Copy link
Contributor Author

master30f commented Jan 3, 2025

Thanks for your feedback.

  • I can certainly bring that back if you wish so.

  • I agree that it's probably a good idea to include the features, but I felt the current list is way too long. I could try and compress it a little bit.

  • I removed this section because I felt like none of the information was actually very important, here's the rundown:

    • vec4 and mat4 variables must be aligned.

    Because this is done automatically by the library, the user does not need to know this (especially a newcomer). In fact this might even cause more confusion (as it did for me), because it sounds like the readme is telling the user they have to align themselves.

    • in and [in, out] parameters must be initialized (please). But [out] parameters not, initializing out param is also redundant

    I think this is unnecessary because it is not something exclusive to this library. This is just common sense C knowledge and anyone who programs in C should know this. If our target audience was people who don't know C, we'd have to explain a lot more things than just this.

    • All functions are inline if you don't want to use pre-compiled versions with glmc_ prefix, you can ignore build process. Just include headers.

    This is explained in Building.

    • if your debugger takes you to cglm headers then make sure you are not trying to copy vec4 to vec3 or alig issues...

    This part might be the only one worth including, but it felt weird to me to have a whole chapter dedicated to a single tip. I would gladly bring it back if you wish so though.

  • I personally think the order is quite intuitive, as it's the same order everyone learns in linear algebra, but it certainly wouldn't hurt to add an example.

Also adding CGLM_OMIT_NS_FROM_STRUCT_API to README could help to discovery this feature: glms_mat4_mul() -> mat4_mul()

That's a good idea, I'd gladly add that too.

To sum it up, i would bring back the slogan, a compressed version of Features, possibly the debugger tip, the example and CGLM_OMIT_NS_FROM_STRUCT_API.

Let me know how you feel about this. If it's good, I'll push an update.

@recp
Copy link
Owner

recp commented Jan 3, 2025

Because this is done automatically by the library,

Not actually. Library do alignment on local variables only ( consider user has float array and pass it to mat4 funcs..., or alignment is enabled on cglm side but disabled in user space... ). If vec4/mat4 is not aligned on user side correctly there will be unexpected results like weird crashes...

For instance

mat4 m1, m2, m3;

glm_mat4_mul(m1, m2, m3);

glm_mat4_mul() expects these matrices to be aligned properly.

vec4 and mat4 have alignment attributes like CGLM_ALIGN_MAT, CGLM_ALIGN_IF(16) but if CGLM_ALL_UNALIGNED is defined then no alignment will be applied, this is optional.

another example:

mat4 m1, m2;
void *m3;
glm_mat4_mul(m1, m2, m3);

m1, m2 is aligned with CGLM_ALIGN_MAT but m3 is unknown by library. User must be aware of alignment issues to fix any unexpected issues. I dont want to hide this info.

This is explained in Building.

Before switching to building, I think we must let them know the design. Inline api, Call api Struct Api, Simd Api...

if your debugger takes you to cglm headers then make sure you are not trying to copy vec4 to vec3 or alig issues...

This part might be the only one worth including, but it felt weird to me to have a whole >chapter dedicated to a single tip. I would gladly bring it back if you wish so though.

I guess this is relevant to alignment I mentioned above.

a compressed version of Features

maybe full list for now :) plus

Thanks

@master30f
Copy link
Contributor Author

@recp I updated the PR, let me know if there's anything else to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants