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

Remove duplicate ARRAY_CUSTOM_RGBA8_UNORM ref in Mesh docs #87973

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

flooxo
Copy link
Contributor

@flooxo flooxo commented Feb 5, 2024

This PR closes godotengine/godot-docs#8901

Removes duplicate reference of ARRAY_CUSTOM_RGBA8_UNORM

@flooxo flooxo requested a review from a team as a code owner February 5, 2024 13:33
@flooxo flooxo changed the title Remove duplicate ref Remove duplicate ARRAY_CUSTOM_RGBA8_UNORM ref Feb 5, 2024
@akien-mga akien-mga changed the title Remove duplicate ARRAY_CUSTOM_RGBA8_UNORM ref Remove duplicate ARRAY_CUSTOM_RGBA8_UNORM ref in Mesh docs Feb 5, 2024
akien-mga
akien-mga previously approved these changes Feb 5, 2024
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good.

@akien-mga akien-mga added this to the 4.3 milestone Feb 5, 2024
@AThousandShips
Copy link
Member

AThousandShips commented Feb 5, 2024

Looking at some of the code I think this should be ARRAY_CUSTOM_RGBA8_SNORM instead

CC @clayjohn

Not sure if this is the code for this but this seems relevant:

const RD::DataFormat fmtrd[RS::ARRAY_CUSTOM_MAX] = { RD::DATA_FORMAT_R8G8B8A8_UNORM, RD::DATA_FORMAT_R8G8B8A8_SNORM, RD::DATA_FORMAT_R16G16_SFLOAT, RD::DATA_FORMAT_R16G16B16A16_SFLOAT, RD::DATA_FORMAT_R32_SFLOAT, RD::DATA_FORMAT_R32G32_SFLOAT, RD::DATA_FORMAT_R32G32B32_SFLOAT, RD::DATA_FORMAT_R32G32B32A32_SFLOAT };

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Feb 5, 2024
@AThousandShips AThousandShips requested a review from a team February 5, 2024 13:39
@akien-mga akien-mga dismissed their stale review February 5, 2024 13:39

Might need to be another constant, see comments.

@kleonc
Copy link
Member

kleonc commented Feb 8, 2024

Looking at some of the code I think this should be ARRAY_CUSTOM_RGBA8_SNORM instead

Indeed, ArrayCustomFormat's docs already say it's one of the encoded as PackedByteArray formats:

godot/doc/classes/Mesh.xml

Lines 264 to 290 in 36e943b

<constant name="ARRAY_CUSTOM_RGBA8_UNORM" value="0" enum="ArrayCustomFormat">
Indicates this custom channel contains unsigned normalized byte colors from 0 to 1, encoded as [PackedByteArray].
</constant>
<constant name="ARRAY_CUSTOM_RGBA8_SNORM" value="1" enum="ArrayCustomFormat">
Indicates this custom channel contains signed normalized byte colors from -1 to 1, encoded as [PackedByteArray].
</constant>
<constant name="ARRAY_CUSTOM_RG_HALF" value="2" enum="ArrayCustomFormat">
Indicates this custom channel contains half precision float colors, encoded as [PackedByteArray]. Only red and green channels are used.
</constant>
<constant name="ARRAY_CUSTOM_RGBA_HALF" value="3" enum="ArrayCustomFormat">
Indicates this custom channel contains half precision float colors, encoded as [PackedByteArray].
</constant>
<constant name="ARRAY_CUSTOM_R_FLOAT" value="4" enum="ArrayCustomFormat">
Indicates this custom channel contains full float colors, in a [PackedFloat32Array]. Only the red channel is used.
</constant>
<constant name="ARRAY_CUSTOM_RG_FLOAT" value="5" enum="ArrayCustomFormat">
Indicates this custom channel contains full float colors, in a [PackedFloat32Array]. Only red and green channels are used.
</constant>
<constant name="ARRAY_CUSTOM_RGB_FLOAT" value="6" enum="ArrayCustomFormat">
Indicates this custom channel contains full float colors, in a [PackedFloat32Array]. Only red, green and blue channels are used.
</constant>
<constant name="ARRAY_CUSTOM_RGBA_FLOAT" value="7" enum="ArrayCustomFormat">
Indicates this custom channel contains full float colors, in a [PackedFloat32Array].
</constant>
<constant name="ARRAY_CUSTOM_MAX" value="8" enum="ArrayCustomFormat">
Represents the size of the [enum ArrayCustomFormat] enum.
</constant>

doc/classes/Mesh.xml Outdated Show resolved Hide resolved
doc/classes/Mesh.xml Outdated Show resolved Hide resolved
doc/classes/Mesh.xml Outdated Show resolved Hide resolved
doc/classes/Mesh.xml Outdated Show resolved Hide resolved
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good now. Great work!

The last step before we merge this is for all the commits to be "squashed" into one commit. We like to have only one commit per PR to keep our git history clean. We have a really helpful tutorial on squashing your commits together in the docs https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

The final result should be just one commit in the PR with a clear commit message

@AThousandShips
Copy link
Member

Please edit your commit message to remove unnecessary messages from commits, should just say:

Remove duplicate reference of CUSTOM_ARRAY_RBGA8_URNOM in docs

@akien-mga akien-mga merged commit 915f176 into godotengine:master Feb 27, 2024
16 checks passed
@akien-mga
Copy link
Member

akien-mga commented Feb 27, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Mar 11, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 11, 2024
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.

ARRAY_CUSTOM_RGBA8_UNORM mentioned twice in a list
5 participants