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

basic PBR example #3621

Merged
merged 4 commits into from
Dec 15, 2023
Merged

basic PBR example #3621

merged 4 commits into from
Dec 15, 2023

Conversation

devdad
Copy link
Contributor

@devdad devdad commented Dec 9, 2023

pbr implementation includes rpbr.h and few shader files header only file, which self contain everything needed for pbr rendering. Few textures and one model of the car which is under free licence which is included inside basic_pbr.c example file currently supported shader versions are 120 and 330 , version 100 has small issue which I have to resolve

pbr implementation  includes rpbr.h and few shader files header only file, which self contain everything needed for pbr rendering. Few textures and one model of the car which is under free licence which is included inside basic_pbr.c example file  currently supported shader versions are 120 and 330 , version 100 has small issue which I have to resolve
I forgot unloading PBRMaterial
@victorfisac
Copy link
Contributor

Thank you for this effort

@raysan5
Copy link
Owner

raysan5 commented Dec 11, 2023

@devdad Amazing! Thank you very much for all the hard work on this great example!

I need some time to review it properly!

@devdad
Copy link
Contributor Author

devdad commented Dec 11, 2023

No problem Ray! It was my pleasure. Please ping me if you need anything.

@devdad
Copy link
Contributor Author

devdad commented Dec 11, 2023

Thank you for this effort

Thank you Victor! It was much easier because of you!

examples/shaders/rpbr.h Outdated Show resolved Hide resolved
value was Vector4 at first but I found out it would be unclear for and users, so I change to have two Vector2 instead, but forgot to assign offset .
@raysan5
Copy link
Owner

raysan5 commented Dec 13, 2023

@devdad Is the plane.glb really needed? Could it be created with raylib directly using GenMeshPlane()?

@raysan5
Copy link
Owner

raysan5 commented Dec 13, 2023

How big are the textures included in pixels? I see they add about 15MB to the repo, that's quite big... maybe texture size could be reduced to 1/4, at least some of them?

@devdad
Copy link
Contributor Author

devdad commented Dec 13, 2023

Hey Ray can we reduce 1/2 and save it as jpg so it would be good quality and smaller? I will reduce textures, but please keep in mind that these models and textures will be used repeatedly in different examples related to PBR.

@devdad
Copy link
Contributor Author

devdad commented Dec 13, 2023

"plane.glb really needed?" Not really, but if we use the GenMeshPlane() we just need to create tangents, it is even better because we will show that we need to generate tangents, otherwise people will be confused why it is not working.

@raysan5
Copy link
Owner

raysan5 commented Dec 13, 2023

Hey Ray can we reduce 1/2 and save it as jpg so it would be good quality and smaller?

If width/2 and height/2 it would be reduced to 1/4, JPG would lose image quality so it's ok to keep them as PNG. Considering it would be used for other examples, maybe only some maps of the car/road can be reduced? It would be nice to make some quick test to see if it is really noticeable the change... Also note that this is an example for the users to see how to implement it, finally visual quality is not that important.

"plane.glb really needed?" Not really, but if we use the GenMeshPlane() we just need to create tangents, it is even better because we will show that we need to generate tangents, otherwise people will be confused why it is not working.

That's fine, you can use GenMeshTangents().

@devdad
Copy link
Contributor Author

devdad commented Dec 13, 2023

yeah agree I will test. I think it would be still good .

@raysan5
Copy link
Owner

raysan5 commented Dec 13, 2023

@devdad I'm checking rpbr.h and I really think it can directly use raylib Model and Material structures but the refactor could be considerable so I will merge it (after the textures size review/naming) and I'll try to work on it myself (also to validate if it is really possible or some parameter needs to be added to raylib structures).

Also noted that at the moment there is no PBREnvironment in use, right?

@devdad
Copy link
Contributor Author

devdad commented Dec 13, 2023

no, for now there is no PBREnvironment but that will come soon IBL, reflection etc. we can use raylib Model and Material but we need to change them a bit, my intention was not to change anything because you already have a lot to do, and PBR can have just about anything in the header, especially since it will be expanded soon
with all the goodies, so maybe it makes sense =) But it's a good idea to at least update the texture enumerations with MRA_TEXTURE and location, because it's really a common implementation to have rgb metallic , roughness , and ao together in one texture.

Changed size of textures from 2048x2048 to 1024x1024 and file name changed to shaders_basic_pbr.c ,
Added the function PBRModel PBRModelLoadFromMesh(Mesh mesh);
but GenMeshPlane(2, 2.0, 3, 3) culdn't be used because it crash once GenMeshTangents() is used with that plane mesh
@devdad
Copy link
Contributor Author

devdad commented Dec 13, 2023

@raysan5 Hey Ray, I did change everything , I changed texture size to 1024x1024 from 2048x2048 , changed the file name to shaders_basic_pbr.c added function PBRLoadModelFromMesh(Mesh mesh); But I couldn't use GenMeshPlane(2.0,2.0,3,3); because plane crash the function GenMeshTangents(); I didn't have time to investigate why, but I did use before GenMeshTangents() on Loaded meshes and it was working.

@raysan5 raysan5 merged commit 9bdc217 into raysan5:master Dec 15, 2023
@raysan5
Copy link
Owner

raysan5 commented Dec 15, 2023

@devdad thank you very much for the addition and the reviews, I'm merging it for further reviews.

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.

4 participants