Skip to content

Conversation

arttu-peltonen
Copy link
Contributor

@arttu-peltonen arttu-peltonen commented Sep 29, 2021

Purpose of this PR

This PR fixes following cases:

The issue seems to be that sometimes AssetDatabase.LoadAssetAtPath(asset, typeof(Material)); can produce a null material, even if asset is not null. This can happen for instance for assets that have the ".mat" suffix but are not actually of type Material.

Testing status

The NullReferenceException was not reproduced locally, so the fix is not verified. Should be minimal risk.

…tPath fails for some reason.

- Example case: if the asset has a ".mat" suffix but it's not a Material.
@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk
With changes to HDRP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

URP
/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

Shader Graph
/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_trunk
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_trunk

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

Copy link
Contributor

@RSlysz RSlysz left a comment

Choose a reason for hiding this comment

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

lgtm

@arttu-peltonen arttu-peltonen marked this pull request as ready for review September 29, 2021 11:39
@arttu-peltonen arttu-peltonen requested review from a team as code owners September 29, 2021 11:39
Comment on lines +337 to +338
if (material == null)
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should threat this an exception or log error. It feels risky we silently fail to process a material.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I'm not sure. As I don't have a repro case, I'm working under the impression that these are not really materials, but some other assets that happen to have a .mat extension. But I'm not sure if that's even allowed or makes sense in any valid use case. Does unity have strict 1:1 mapping between asset file extensions and asset types? Perhaps this needs some deeper investigation...

@arttu-peltonen arttu-peltonen marked this pull request as draft September 30, 2021 05:47
@arttu-peltonen
Copy link
Contributor Author

arttu-peltonen commented Sep 30, 2021

@Unity-Technologies/gfx-qa-urp I've returned this PR to draft status as per @phi-lira's comment I'm not sure anymore if this is a safe fix, if it might silently leave some materials unprocessed. Could you help me find someone to reproduce the original issue so we can understand what actually causes it? I was unable to find a SG QA person to contact since the original reporter has left.

Update: I was able to repro this finally on 21.2.

@arttu-peltonen
Copy link
Contributor Author

Upon further examination and discussion with @RSlysz, it seems like the fix proposed here is not the correct one. The reason for LoadAssetAtPath returning null seems to be related to AssetDatabase internals, and Rémi has separate ongoing work concerning that, so I'm going to close this PR and pass the bug to him. The issue seems to be connected to this bug: https://fogbugz.unity3d.com/f/cases/1347209/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants