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

WIP fix: make hue non nullable #2346

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IXLLEGACYIXL
Copy link
Collaborator

PR Details

makes property hue non nullable as its a non allowed type for attribute properties.

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

There is no usage in stride of that field thats why it was unnoticed when it was made nullable.
I dont know how to test it out. So i need help someone knowing how to test that out in a project.

@Fydar
Copy link
Contributor

Fydar commented Jun 23, 2024

Technically is a breaking change as float is a value type, so the compiler previously used Nullable<float> for the property type, and that's now becoming a float.

Looks good to me though. I doubt that's going to affect anyone.

@IXLLEGACYIXL
Copy link
Collaborator Author

Technically is a breaking change as float is a value type, so the compiler previously used Nullable<float> for the property type, and that's now becoming a float.

Looks good to me though. I doubt that's going to affect anyone.

previous version didnt even compile if you wanted to set it
image

as only primitives are supported, nullable isnt a primitive

@Doprez
Copy link
Contributor

Doprez commented Jun 23, 2024

The good news is this does let me use the CustomHue property but it doesnt seem to change anything in the editor.

image

[Display("Test", CustomHue = 100f)]
public class Test : SyncScript
{

	[Display("ModelComponent", CustomHue = 250f)]
	public ModelComponent ModelComponent { get; set; }
}

@IXLLEGACYIXL IXLLEGACYIXL changed the title fix: make hue non nullable WIP fix: make hue non nullable Jun 23, 2024
@IXLLEGACYIXL IXLLEGACYIXL marked this pull request as draft June 23, 2024 23:13
@Doprez
Copy link
Contributor

Doprez commented Jun 24, 2024

This somehow breaks the dynamic colour of the Raw file thumbnails.'

uncompressed thumbnail before change
image

compressed thumbnail before change
image

with this change the thumbnail stays a single colour for some reason with or without a proper value:
image

@Basewq
Copy link
Contributor

Basewq commented Jul 16, 2024

This somehow breaks the dynamic colour of the Raw file thumbnails.'

That's because CustomHue's function is to affect the thumbnail, not the property header (which kinda makes this feature a bit useless)

It's only used here:

BackgroundColor = (Color)assetType.GetUniqueColor();

Which is where the thumbnail asset then uses as the color:
SpriteBatch.Draw(BackgroundTexture, new RectangleF(0, 0, thumbnailSize.X, thumbnailSize.Y), null, BackgroundColor, 0, Vector2.Zero, SpriteEffects.None, ImageOrientation.AsIs, 0, AdditiveColor, Swizzle);

The thumbnail only persists as a single color (for a given asset) because displayAttribute?.Name.GetHashCode() or type.Name.GetHashCode() are getting hashcodes from strings which are not persistent hashcodes between different sessions (though should persists in a single app session).

As for whether CustomHue ever affected the property header...? Not sure it did?
If not, in my opinion a hex string property would be more user friendly instead of using this.

As an FYI, the background color of the header is set here (which is currently not changeable):

<Border x:Name="PART_Name" MinHeight="20" Margin="6" Background="{StaticResource EmphasisColorBrush}"

@IXLLEGACYIXL
Copy link
Collaborator Author

so is that property just useless in the end? seems like a leftover

if oyu set hue color the color stays the same for the thumbnail sooo its anyway pointless as the intention is different?

@Basewq
Copy link
Contributor

Basewq commented Jul 17, 2024

Very low value since none of the existing assets use it.
I would point out that it is technically possible to inherit Asset (or AssetWithSource) and set the Display -> ColorHue for your own custom asset, however creating your own custom asset that appears in the editor involves a lot of setup, so I don't think anyone is aware of this or does this.

@IXLLEGACYIXL
Copy link
Collaborator Author

currently setting hue value doesnt compile, and nobody complained

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.

4 participants