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

Fix issue where material instance name cannot be set without creating a default instance #8213

Merged
merged 6 commits into from
Oct 22, 2024
Merged

Fix issue where material instance name cannot be set without creating a default instance #8213

merged 6 commits into from
Oct 22, 2024

Conversation

kunyoungparkk
Copy link
Contributor

This PR is related to PR #8147

After applying PR #8147, FMaterial::createInstance has an issue where it ignores the name passed as a parameter when mDefaultMaterialInstance is not set.

By applying this PR, a material instance can be assigned a name even if mDefaultMaterialInstance is not set.

Copy link

google-cla bot commented Oct 18, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@pixelflinger pixelflinger added the internal Issue/PR does not affect clients label Oct 18, 2024
@pixelflinger
Copy link
Collaborator

@kunyoungparkk would you be able to sign the CLA so we can merge this change? Thank you!

FMaterialInstance* p = mHeapAllocator.make<FMaterialInstance>(*this, material);
FMaterialInstance* FEngine::createMaterialInstance(const FMaterial* material,
const char* name) noexcept {
FMaterialInstance* p =
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 4 spaces here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed spacing as suggested. Thanks for pointing that out!

@@ -53,7 +53,8 @@ class FTexture;

class FMaterialInstance : public MaterialInstance {
public:
FMaterialInstance(FEngine& engine, FMaterial const* material) noexcept;
FMaterialInstance(FEngine& engine, FMaterial const* material,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spacing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed spacing as suggested. Thanks for pointing that out!

@pixelflinger
Copy link
Collaborator

@kunyoungparkk windows build is failing

@kunyoungparkk
Copy link
Contributor Author

@kunyoungparkk windows build is failing

I’m currently investigating the issue as the build is working fine in my local Windows environment. It might take some time to identify the root cause. I appreciate your patience, and I will provide an update once the issue is resolved.

@pixelflinger pixelflinger merged commit 13310e4 into google:main Oct 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants