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: Update Texture reload to prevent crash due to wrong type. #2251

Merged
merged 6 commits into from
May 18, 2024

Conversation

Doprez
Copy link
Contributor

@Doprez Doprez commented May 16, 2024

PR Details

when reloading a Texture it would try to pass in a value of type Image to the content manager for an asset that of type Texture.

Description

This adds a new method to the Content loader to check the type of an asset from the URL.

This also adds that check to the Texture class to determine how it should be treated when reloading.

Related Issue

#2008
#1493

Motivation and Context

Its the start of a fix for a seemingly larger problem with disposing of the graphics device when the game is in fullscreen.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@Doprez
Copy link
Contributor Author

Doprez commented May 16, 2024

I was also debating if there should be the possibility to prevent crashing on failed Asset loading by adding a try catch but I know its best to avoid if possible.

        public T Load<T>(string url, ContentManagerLoaderSettings settings = null) where T : class
        {
            try
            {
                return (T)Load(typeof(T), url, settings);
            }
            catch(Exception ex)
            {
                Log.Error($"Error loading asset '{url}': {ex.Message}");
                return null;
            }
        }

instead of

        public T Load<T>(string url, ContentManagerLoaderSettings settings = null) where T : class
        {
            return (T)Load(typeof(T), url, settings);
        }

Copy link
Collaborator

@Jklawreszuk Jklawreszuk left a comment

Choose a reason for hiding this comment

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

Looks legit 😄

@Eideren
Copy link
Collaborator

Eideren commented May 18, 2024

This is far more efficient:

var textureDataReloaded = assetManager.Load<object>(url);
if (textureDataReloaded is Texture tex)
   // ...
else if (textureDataReloaded is Image img)
   // ...

As for the method signature you introduced in ContentManager - if we were to have a method for this purpose, I think it's best to have a bool TryGetLoadedAsset(string myUrl, out object myAsset) and then the user would test the asset's type if that's what they're looking for, but they could also just use the asset itself as an added feature. But this would effectively be the same as calling IsLoaded(url) followed by Load<object>(url) and testing the type like shown above. So all in all I think it's best to remove public Type GetType(string url) and we could instead document the fact that Load<T> works with a derived type.

@Doprez
Copy link
Contributor Author

Doprez commented May 18, 2024

Updated based on the suggestion. I agree with the documentation especially, I had no clue you could just use the base type object even though it makes perfect sense thining about it now.

@Eideren Eideren changed the title Update Texture reload to prevent crash due to wrong type. fix: Update Texture reload to prevent crash due to wrong type. May 18, 2024
@Eideren Eideren merged commit e728af2 into stride3d:master May 18, 2024
2 checks passed
@Eideren
Copy link
Collaborator

Eideren commented May 18, 2024

Thanks !

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.

3 participants