Skip to content

Conversation

@neptuwunium
Copy link
Contributor

Copy link
Contributor

@ds5678 ds5678 left a comment

Choose a reason for hiding this comment

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

A lot to unpack with this, despite the small line count.

  • The RBAConverter.{format name}ToBGRA32 methods are legacy code and scheduled to be removed at some point. This is partially my fault; I should have added obsolete attributes to signal my intent. These methods have been replaced with structs in the Formats namespace. Your current method can stay for use in unit tests, ie RGBTests and NewRGBTests.
  • I'm concerned about the use of Half and float. Given that the significand for Half uses 10 bits not including the sign bit (reference 1, reference 2), this is actually just an integer. The single exponent bit for R and G could only double the significand, the same as normal integer types. It should be more efficient to use ushort directly (similar to RGB16). You should verify all this with unit tests and document accordingly. Assuming my thoughts are correct, I want this to be RGB32 : IColor<ushort>, otherwise RGB32Half : IColor<Half>.
  • You should add a line here and run automated test generation for the new color struct.

@neptuwunium
Copy link
Contributor Author

f16 still has 5 exponent bits, i.e. 0x3401 is 0.2503~ there's no neat conversion to a UNORM value (0x4013 as U16, or 0x3F as U8) without doing what Half does anyway.

Copy link
Contributor

@ds5678 ds5678 left a comment

Choose a reason for hiding this comment

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

Much closer this time. I would appreciate a test file, but that's not strictly necessary. I verified that this is a format used by Unity, albeit only for render textures.

@neptuwunium neptuwunium requested a review from ds5678 November 7, 2022 03:41
@neptuwunium
Copy link
Contributor Author

Rebased the code and fixed the comments/remarks.

Copy link
Contributor

@ds5678 ds5678 left a comment

Choose a reason for hiding this comment

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

One small change and then I think this is good, assuming the continuous integration succeeds.

@neptuwunium
Copy link
Contributor Author

Alright, added the test case using RGB24's sample file. I should probably dump the r11g11b10 texture i was using and provide that but this works too.

@ds5678
Copy link
Contributor

ds5678 commented Nov 7, 2022

I should probably dump the r11g11b10 texture i was using and provide that

If licensing is not an issue, I would greatly appreciate this.

@neptuwunium
Copy link
Contributor Author

Unsure on licensing. I'll probably make a new one someday and PR it with the updated test case.

@ds5678
Copy link
Contributor

ds5678 commented Nov 7, 2022

Sounds good!

@ds5678 ds5678 merged commit b4ad9bc into AssetRipper:master Nov 7, 2022
ds5678 added a commit that referenced this pull request Aug 2, 2023
* Resolves #42
* Removes many of the nongeneric structs
* Unit tests for ColorRGB32Half are disabled (#17)
@ds5678
Copy link
Contributor

ds5678 commented Aug 2, 2023

@yretenai I think there may be bugs in this color. I had to disable the unit tests for it in the above commit. It is the only color with failing unit tests.

@ds5678
Copy link
Contributor

ds5678 commented Aug 2, 2023

If you're not willing to fix the bugs, I would like to remove this color from the codebase, for quality control. This would not impact your ability to use the library. I specifically designed it to be easy for someone to declare their own custom structs inheriting from IColor<T>.

@neptuwunium
Copy link
Contributor Author

I specifically designed it to be easy for someone to declare their own custom structs inheriting from IColor.

Feel free to remove it in this case, I'll add it to my own use cases.

ds5678 added a commit that referenced this pull request Aug 2, 2023
@ds5678
Copy link
Contributor

ds5678 commented Aug 2, 2023

Thank you for understanding.

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.

2 participants