Skip to content

Multiple cgltf Nodes with base_color_texture Import, Skysphere matrix function#101

Open
TimeTravelerFromNow wants to merge 15 commits into
MrFrenik:masterfrom
TimeTravelerFromNow:master
Open

Multiple cgltf Nodes with base_color_texture Import, Skysphere matrix function#101
TimeTravelerFromNow wants to merge 15 commits into
MrFrenik:masterfrom
TimeTravelerFromNow:master

Conversation

@TimeTravelerFromNow
Copy link
Copy Markdown

@TimeTravelerFromNow TimeTravelerFromNow commented Jul 24, 2024

Sky matrix and cgltf material mapping

This PR expands cgltf loading; pbr basic color texture image url is used to load a texture and is saved in a pbr_textures array. The cgltf node's material pointer is used to map to save a material id on the mesh primitive in gunslinger.

Materials are initialized and a handle to the corresponding material is stored on the pbr_node for rendering the correct material.
Currently this only supports the base_color_texture.

sky matrix

The sky matrix functions are matrix constructors that ignore position calculations, and can be used for skyboxes, skyspheres etc.

Todos copied from the example Pull Request

  • Matrix transforms for child- parent nodes.
  • More pbr textures and flag handling as in the gltf spec
  • the black mesh actually has a vec4 base color that isn't rendered yet.
  • Expand upon the pbr_basic.sf pipeline

Can handle some basic material conversion to the pbr struct. Needs slot_map implemented for rendering scenes.
We loaded the gltf file and got the index 0 to test placing into scene nodes dyn_array
Proof of concept.
Scene renders all the meshes with base color textures.
Also we don't bind the texture every draw call. Didn't seem to change smoothness that much (my computer has decent specs)
@Samdal
Copy link
Copy Markdown
Contributor

Samdal commented Jul 24, 2024

gs.h

What's the incentive behind the sky matrix functions? Just set the position to zero before getting the projection matrix.

    gs_camera_t tmp_cam = *cam;
    tmp_cam.transform.position = gs_v3s(0);
    gs_mat4 svp = gs_camera_get_view_projection(&tmp_cam, fbs.x, fbs.y);

All of those 60 lines don't do anything useful unless I'm misunderstanding something.


gs_gfxt

gs_gfxt_load_gltf_all_data_from_file

With this PR there will be two functions to load gltf data, why not update the existing one?

gs_gfxt_scene_new

As you mentioned in the other PR, this function uses hardcoded paths. Which isn't acceptable.
The function also doesn't do anything. If I rewrite the function it becomes easier to see why

GS_API_DECL 
gs_gfxt_scene_t gs_gfxt_scene_new()
{
    return (gs_gfxt_scene_t){
        .pbr_pip = gs_gfxt_pipeline_load_from_file("path/to/pipeline")
    };
}

The user should just call gs_gfxt_pipeline_load_from_file themselves, no need to create a new function.
Even if the pbr pipeline was to be vendored with gfxt using gs_gfxt_pipeline_load_from_memory, having a function like this would be better.

GS_API_DECL gs_gfxt_pipeline_t gs_gfxt_default_pbr_pipeline(void);

gs_gfxt_scene_pbr_draw

This function is mostly alright. but going with the idea of simply providing a default pipeline, renaming it to something like gs_gfxt_default_pbr_pipeline_draw would show clearly that it's specific to that pipeline.

gs_gfxt_scene_free

Your free function should also destroy the items contained within the slot arrays.

@TimeTravelerFromNow
Copy link
Copy Markdown
Author

Glad to start this discussion on the items brought up.

gs.h

sky matrix functions

Why not just set the position column zero?
This discards the last column of a standard view_projection matrix operation; for sure the easiest solution.
The usefulness of sky-specific matrix, parts of the operation never need to be done in the first place.
I can argue at least unlike the regular perspective matrix maker: gs.h line 3958, that we can omit 3 dot products.
Meaning gs_gfxt_sky_perspective doesn't need these:

    m_res.elements[3 * 4 + 0] = -gs_vec3_dot(s, position);;
    m_res.elements[3 * 4 + 1] = -gs_vec3_dot(u, position);
    m_res.elements[3 * 4 + 2] = gs_vec3_dot(f, position); 

and gs_gfxt_sky_perspective doesn't need this
f32 c = (2.0f * n * f) / (n - f);
I agree less lines of code is definitely better.
What do you think @Samdal ?

gs_gfxt

gs_gfxt_load_gltf_all_data_from_file

started with a copy of the original. Didn't know if it was going to deviate a lot from the other gs_gfxt_load_gltf_data_from_file. Can for sure be merged with the original function.

gs_gfxt_scene_new

Your comment was the idea I needed input on this. If I understand right, scene_new will allow an optional user pipeline file, else it loads_from_memory the vendored default pbr pipeline. And the function example you wrote I'll add that.

Can you give me some help on how I would accomplish vendoring the pipeline from memory?

gs_gfxt_scene_pbr_draw

gs_gfxt_default_pbr_pipeline_draw. sounds good

gs_gfxt_scene_free

Definetely. Got some free functions to write

Future ideas after current work is done

Imagining what sort of physically-based rendering meshes we can draw will be so sick. Also I did some digging into gltf collision physics extensions, KHR_collision_shapes and KHR_physics_rigid_bodies, OMI_physics_body all super cool. KHR_physics_rigid_bodies shapes has a working blender extension.

Much appreciated for the review Samdal.

Comment thread gs.h Outdated
}

gs_inline gs_mat4
gs_mat4_sky_look_at(gs_vec3 position, gs_vec3 target, gs_vec3 up)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The position argument is redundant. Remove it

Comment thread gs.h Outdated
gs_inline gs_mat4
gs_mat4_sky_look_at(gs_vec3 position, gs_vec3 target, gs_vec3 up)
{
gs_vec3 f = gs_vec3_norm(gs_vec3_sub(target, position));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove subtraction.
I think target should already be normalized? I'm not completely sure

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Looked around for camera rotation vectors used by gs_camera_forward. They are unit vectors!

Comment thread gs.h Outdated
{
gs_vec3 up = gs_camera_up(cam);
gs_vec3 forward = gs_camera_forward(cam);
gs_vec3 target = gs_vec3_add(forward, cam->transform.position);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove this line and provide forward as the target. Addition is redundant.

Comment thread gs.h Outdated
gs_vec3 up = gs_camera_up(cam);
gs_vec3 forward = gs_camera_forward(cam);
gs_vec3 target = gs_vec3_add(forward, cam->transform.position);
return gs_mat4_sky_look_at(cam->transform.position, target, up);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

update to not provide cam->transform.position. Provide forward directly

Comment thread gs.h Outdated
GS_API_DECL gs_mat4 gs_camera_get_view(const gs_camera_t* cam);
GS_API_DECL gs_mat4 gs_camera_get_proj(const gs_camera_t* cam, int32_t view_width, int32_t view_height);
GS_API_DECL gs_mat4 gs_camera_get_view_projection(const gs_camera_t* cam, int32_t view_width, int32_t view_height);
GS_API_DECL gs_mat4 gs_camera_get_sky_view_projection(const gs_camera_t* cam, int32_t view_width, int32_t view_height);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add comment // Camera position is ignored

@Samdal
Copy link
Copy Markdown
Contributor

Samdal commented Jul 25, 2024

gs_gfxt_scene_new

Your comment was the idea I needed input on this. If I understand right, scene_new will allow an optional user pipeline file, else it loads_from_memory the vendored default pbr pipeline. And the function example you wrote I'll add that.

Can you give me some help on how I would accomplish vendoring the pipeline from memory?

You didn't exactly understand what I meant.

Your function:

GS_API_DECL 
gs_gfxt_scene_t gs_gfxt_scene_new()
{
  // maybe we can ship this standard pipeline with the gs framework
  gs_println("looking for pbr_basic in ./assets/pipelines/pbr_basic.sf");
  const char* pbr_basic_path = "./assets/pipelines/pbr_basic.sf"; 
  gs_gfxt_pipeline_t pbr_pip = gs_gfxt_pipeline_load_from_file(pbr_basic_path);
  gs_gfxt_scene_t scene = {0};
  scene.pbr_pip = pbr_pip;
  return scene;
};

usage of your function:

gs_gfxt_scene_t scene = gs_gfxt_scene_new()

my function:

// it doesn't exist

usage of my function:

gs_gfxt_scene_t scene = {.pbr_pip = gs_gfxt_pipeline_load_from_file("path/to/pipeline")};

If you were to vendor a pipeline in together with gfxt, you should do this

GS_API_DECL 
gs_gfxt_pipeline_t gs_gfxt_default_pbr_pipeline(void)
{
  const char* pip = "/*\n"
"    TODO(john): Need a description of the .sf format here\n"
"*/\n"
"\n"
"pipeline {\n"
"\n"
"    raster\n"
"    {\n"
"        primitive: TRIANGLES\n"
"        index_buffer_element_size: UINT32\n"
"    },\n"
"\n"
"    depth\n"
"    {\n"
"        func: LESS\n"
"    },\n"
"\n"
"\n"
"    shader {\n"
"\n"
"        vertex {\n"
"\n"
"            attributes {\n"
"\n"
"                /*\n"
"                    Vertex layout required for this pipeline (for input assembler)\n"
"\n"
"                    fields:\n"
"\n"
"                    POSITION: 	      	float3\n"
"                    TEXCOORD: 	      	float2\n"
"                    TEXCOORD[0 - 12]: 	float2\n"
"                    COLOR:    		uint8[4]\n"
"                    NORMAL:   		float3\n"
"                    TANGENT:  		float4\n"
"                    JOINT:    		float4\n"
"                    WEIGHT:   		float\n"
"                    FLOAT: 		float\n"
"                    FLOAT2: 		float2\n"
"                    FLOAT3: 		float3\n"
"                    FLOAT4: 		float4\n"
"                */\n"
"\n"
"                POSITION : a_position\n"
"                TEXCOORD : a_uv\n"
"                COLOR    : a_color\n"
"            },\n"
"\n"
"            uniforms {\n"
"                mat4 u_mvp;\n"
"            },\n"
"\n"
"            out {\n"
"                vec2 uv;\n"
"                vec3 position;\n"
"            },\n"
"\n"
"            code {\n"
"                void main() {\n"
"                    gl_Position = u_mvp * vec4(a_position, 1.0);\n"
"                    uv = a_uv;\n"
"                    position = a_position;\n"
"                }\n"
"            }\n"
"        },\n"
"\n"
"        fragment {\n"
"        \n"
"            uniforms {\n"
"                sampler2D u_base_col_tex;\n"
"                vec4 u_base_col_fact;\n"
"            },\n"
"            \n"
"            out {\n"
"                vec4 frag_color;\n"
"            },\n"
"\n"
"            code {\n"
"                void main() {\n"
"                    frag_color = texture(u_base_col_tex, uv); // //\n"
"                }\n"
"            }\n"
"        }\n"
"    }\n"
"}\n";

  return gs_gfxt_pipeline_load_from_memory(pip, sizeof(pip)-1);
};

and then usage of it

gs_gfxt_scene_t scene = {.pbr_pip = gs_gfxt_default_pbr_pipeline()};

@Samdal
Copy link
Copy Markdown
Contributor

Samdal commented Jul 25, 2024

Future ideas after current work is done

Imagining what sort of physically-based rendering meshes we can draw will be so sick.

The current PBR pipeline here only does color texture mapping. So it can't really do much in the current state, calling it PBR is a bit misleading to be honest. You'll have to do quite a bit more work for it to be called that, but I'm guessing that's the end goal so it doesn't bother me.

@TimeTravelerFromNow TimeTravelerFromNow marked this pull request as draft July 26, 2024 03:19
@TimeTravelerFromNow
Copy link
Copy Markdown
Author

Added the free of arrays and merged the material loading logic into the existing gs_gfxt_mesh_load_from_file function, which had to be changed to accept the mesh directory, so we can load image URIs from the containing folder.

I wanted to set fragment uniforms to pass in has_base_color_factor, however gs_gfxt_uniform_block_create line 596 has the existing types, no custom structs yet. Thinking of adding a struct for the pbr description.

@TimeTravelerFromNow TimeTravelerFromNow marked this pull request as ready for review July 28, 2024 00:34
@Samdal
Copy link
Copy Markdown
Contributor

Samdal commented Jul 29, 2024

I would've probably not had util/pbr_default.h as it's own file, and rather have it be inside util/gs_gfxt.h, but that's mostly personal preference.
At this point it's mostly up to what @MrFrenik thinks about the PR.

@TimeTravelerFromNow
Copy link
Copy Markdown
Author

Great, lmk if I should squash the commits

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.

2 participants