Skip to content

convert texture* from i32 to u31 in TextRendering - #3615

Open
realugi wants to merge 1 commit into
PixelGuys:masterfrom
realugi:fix/3594
Open

realugi wants to merge 1 commit into
PixelGuys:masterfrom
realugi:fix/3594

Conversation

@realugi

@realugi realugi commented Sep 19, 2026

Copy link
Copy Markdown

fix #3594

I chose to use u31 instead of u32, because u31 is implicitly convertable to i32 and therefore c_int. Also saw precedence in main.graphics.Image.

Also removed the now unneeded cast at L1390, because both bitmap.width and textureOffset are now unsigned integers.

@Wunka Wunka moved this to Easy to Review in PRs to review Sep 19, 2026

@IntegratedQuantum IntegratedQuantum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason to support c_int here? Will it even be needed once we get rid of OpenGL after #102?

@IntegratedQuantum IntegratedQuantum moved this from Easy to Review to In review in PRs to review Sep 19, 2026
@realugi

realugi commented Sep 19, 2026

Copy link
Copy Markdown
Author

So the reason to support c_int in OpenGL case is essentially so that we do not need to do any conversions when passing our values to the OpenGL API.

The same applies to Vulkan (in some cases):

  • for the TextRendering.init vulkan path, the vulkan.Image.init method accepts main.vec.Vec3i (which contains i32s), but the actual vulkan api accepts VkExtent2D, which contains u32s. So in this case, u32s for texture{Width|Height} would suffice. Though we'd need to adjust the vulkan.Image.init method params if we wanna do this cleanly.
  • for the TextRendering.uploadData vulkan path, vulkan.Image.uploadData method accepts a VkOffset3D (through vulkan.Image.UploadDataConfig's .imageOffset), which contains i32s. So in this case, making textureOffset a u32 would require a conversion.

So if we want texture{Width|Height|Offset} types to fit the vulkan api, then:

  • texture{Width|Height} should be u32, w/ conversions for opengl, w/o conversions for vulkan, though I'd need to adjust the vulkan.Image.init's size: main.vec.Vec3i parameter, which also maybe necessitates changin the size field of vulkan.Image, which could become quite an extensive change actually
  • textureOffset stays u31

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

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

TextRendering.textureWidth/-Height/-Offset should be unsigned integers

3 participants