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.

Sign up for GitHub

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

Jump to bottom

Add more physics options to the Scene importer #77533

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Add more physics options to the Scene importer #77533

merged 1 commit into from
Aug 1, 2023

Conversation

EMBYRDEV
Copy link
Contributor

@EMBYRDEV EMBYRDEV commented May 26, 2023 •
edited by YuriSizov
Loading

Implements and closes godotengine/godot-proposals#6932

image

@EMBYRDEV EMBYRDEV requested a review from a team as a code owner May 26, 2023 22:33
@EMBYRDEV EMBYRDEV force-pushed the phys-mat-import-option branch from 3bef421 to c0d1edc Compare May 26, 2023 22:40
@Calinou Calinou added this to the 4.x milestone May 26, 2023
@EMBYRDEV EMBYRDEV force-pushed the phys-mat-import-option branch from c0d1edc to 8c2f952 Compare May 26, 2023 22:47
@EMBYRDEV
Copy link
Contributor Author

I should probably also add the option to set the collision layer and mask. These are important for the same reason exposing PMO is (not having to create hundreds of inherited scenes just to configure things properly)

@EMBYRDEV
Copy link
Contributor Author

image

Done!

@EMBYRDEV EMBYRDEV changed the title Add Physics Material Override option to Scene Importer. Add Additional Physics Options to Scene Importer. May 26, 2023
@EMBYRDEV
Copy link
Contributor Author

After some additional discussion, I'm going to expose some additional options on meshes too to streamline the import process.

  • Cast Shadow
  • Lightmap Scale
  • LOD Bias
  • Visibility Ranges
  • Layer

@fire
Copy link
Member

fire commented May 27, 2023

I want to support this, but trying to get people to review.

@EMBYRDEV
Copy link
Contributor Author

I want to support this, but trying to get people to review.

It's not quite ready for review yet since speaking to Calinou I plan on adding a bunch of extra options.

That being said Calinou seems to be on board but I'm not sure if I'll need additional approval for this to be pulled in.

@fire
Copy link
Member

fire commented May 27, 2023

Can you consider splitting the feature? It's easier to merge smaller changes.

@EMBYRDEV
Copy link
Contributor Author

Can you consider splitting the feature? It's easier to merge smaller changes.

Sure thing, In that case please feel free to get this change reviewed as I think the physics options belong in a single PR.

@EMBYRDEV EMBYRDEV force-pushed the phys-mat-import-option branch from e6e491a to adb86dd Compare May 27, 2023 22:41
@EMBYRDEV
Copy link
Contributor Author

EMBYRDEV commented Jun 2, 2023

@Calinou any movement on this?

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2023 •
edited
Loading

We're in feature freeze now, and the milestone is 4.x, so I think this might have to wait for 4.2

@EMBYRDEV EMBYRDEV mentioned this pull request Jun 28, 2023
Merged
@EMBYRDEV
Copy link
Contributor Author

Is there anything else needing done before looking at getting this merged?

Calinou
Calinou approved these changes Jul 16, 2023
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 851bc64), it works as expected.

Testing project: test_pr_77533.zip

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 25, 2023
@YuriSizov YuriSizov requested review from a team July 25, 2023 15:23
rburing
rburing approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@rburing rburing left a comment

Choose a reason for hiding this comment

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

Makes sense. Makes the advanced importer a lot more useful with just a tiny bit of code.

editor/import/resource_importer_scene.cpp
@@ -1245,6 +1245,10 @@ Node *ResourceImporterScene::_post_fix_node(Node *p_node, Node *p_root, HashMap<
col->set_owner(p_node->get_owner());
col->set_transform(get_collision_shapes_transform(node_settings));
col->set_position(p_applied_root_scale * col->get_position());
const Ref<PhysicsMaterial> &pmo = node_settings["physics/physics_material_override"];
if (!pmo.is_null()) {
Copy link
Member

Choose a reason for hiding this comment

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

This could be if (pmo.is_valid()) (same applies to the other cases), but no biggie.

godot/core/object/ref_counted.h

Lines 201 to 202 in 202e4b2

inline bool is_valid() const { return reference != nullptr; }
inline bool is_null() const { return reference == nullptr; }

@YuriSizov YuriSizov changed the title Add Additional Physics Options to Scene Importer. Add more physics options to the Scene importer Aug 1, 2023
@YuriSizov YuriSizov merged commit 266e195 into godotengine:master Aug 1, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@knil79 knil79 mentioned this pull request Oct 16, 2023
Open
@EMBYRDEV EMBYRDEV deleted the phys-mat-import-option branch November 11, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rburing rburing rburing approved these changes

@Calinou Calinou Calinou approved these changes

Assignees
No one assigned
Labels
enhancement topic:import topic:physics topic:3d
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Add "Physics Material Override" option for generated bodies in the Advanced Import Settings dialog
6 participants
@EMBYRDEV @fire @AThousandShips @YuriSizov @Calinou @rburing