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

Add clip and skip (nodraw) texture support #29

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

EIREXE
Copy link
Contributor

@EIREXE EIREXE commented Sep 29, 2022

Adds clip and nodraw texture support, as a property to TBLoader

Copy link
Owner

@codecat codecat left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have some comments.

src/builder.cpp Outdated
@@ -447,6 +448,10 @@ MeshInstance3D* Builder::build_entity_mesh(int idx, LMEntity& ent, Node3D* paren
// Create material
Ref<Material> material;

if (String(tex.name) == m_loader->get_skip_texture_name()) {
Copy link
Owner

Choose a reason for hiding this comment

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

It would probably be good to have a comment here explaining what this is doing (even if it's clear by looking at the code)

Copy link
Owner

Choose a reason for hiding this comment

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

Additionally, casting tex.name to a String might invoke a memory allocation & string copy for each texture, we can probably avoid that? (Doesn't String have a compare method against const char*?)

src/builder.cpp Outdated
Comment on lines 486 to 492
add_surface_to_mesh(mesh, surf);
add_surface_to_mesh(collision_mesh, surf);

// Give mesh material
if (material != nullptr) {
mesh->surface_set_material(mesh->get_surface_count() - 1, material);
// Add surface to mesh
Copy link
Owner

Choose a reason for hiding this comment

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

Misplaced comment? These should ultimately have 2 different comments, one for adding the surface to the visible mesh, and one for adding the surface to the collision mesh.

src/builder.cpp Outdated
if (material != nullptr) {
mesh->surface_set_material(mesh->get_surface_count() - 1, material);
// Add surface to mesh
if (String(tex.name) != m_loader->get_clip_texture_name()) {
Copy link
Owner

Choose a reason for hiding this comment

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

It would be cleaner to do an if / continue here I think

Comment on lines +516 to +524
StaticBody3D* static_body = memnew(StaticBody3D());
static_body->set_name(String(mesh_instance->get_name()) + "_col");
parent->add_child(static_body, true);
static_body->set_owner(m_loader->get_owner());
add_collider_from_mesh(static_body, collision_mesh, colshape);
Copy link
Owner

Choose a reason for hiding this comment

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

Does this change the behavior from mesh_instance->create_multiple_convex_collisions() or mesh_instance->create_trimesh_collision()?

Comment on lines 124 to 143
void TBLoader::set_clip_texture_name(String clip_texture_name) {
m_clip_texture_name = clip_texture_name;
}

String TBLoader::get_clip_texture_name() {
return m_clip_texture_name;
}

void TBLoader::set_skip_texture_name(String skip_texture_name) {
m_skip_texture_name = skip_texture_name;
}

String TBLoader::get_skip_texture_name() {
return m_skip_texture_name;
}

Copy link
Owner

Choose a reason for hiding this comment

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

{ should be on a new line here. Also I think we can pass the strings as const String&, no?

@EIREXE EIREXE force-pushed the clip_nodraw branch 2 times, most recently from 5c8905f to c0621c0 Compare September 30, 2022 06:07
@EIREXE
Copy link
Contributor Author

EIREXE commented Sep 30, 2022

all fixed

@codecat codecat merged commit b3bdeef into codecat:master Sep 30, 2022
@codecat
Copy link
Owner

codecat commented Sep 30, 2022

Thank you! 🎉

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