feat: implemented create and update texture #2

Merged
LordMZTE merged 4 commits from coding-agent/zenolith-sdl2:feature/create-texture into master 2024-03-18 23:16:49 +01:00
Contributor

Hi,

Made a few changes in zenolith-sdl2 platform and texture to be able to create texture and update it. Please review it and let me know if any changes are required.

Hi, Made a few changes in zenolith-sdl2 platform and texture to be able to create texture and update it. Please review it and let me know if any changes are required.
coding-agent added 1 commit 2024-03-18 18:15:20 +01:00
LordMZTE requested changes 2024-03-18 18:42:18 +01:00
LordMZTE left a comment
Owner

A decent first try knowing you, although there's a lot of improvements to be done.
Also change Sdl2Platform.createFont such that it uses createFont and Sdl2Font.addAtlasSprite such that it uses updateTexture. Thanks!

A decent first try knowing you, although there's a lot of improvements to be done. Also change `Sdl2Platform.createFont` such that it uses `createFont` and `Sdl2Font.addAtlasSprite` such that it uses `updateTexture`. Thanks!
@ -366,3 +366,3 @@
const atlas = c.SDL_CreateTexture(
self.renderer,
c.SDL_PIXELFORMAT_RGBA8888,
c.c.SDLPIXELFORMAT_RGBA8888,
Owner

WTF (this doesn't even compile)

WTF (this doesn't even compile)
LordMZTE marked this conversation as resolved
@ -401,2 +401,4 @@
});
}
const PixelAccess = enum {
Owner

Turn this into an enum(c_int) and remove any switches converting it to the respective SDL2 value.

Also move it into Sdl2Texture and mark it pub

Turn this into an `enum(c_int)` and remove any `switch`es converting it to the respective SDL2 value. Also move it into `Sdl2Texture` and mark it `pub`
LordMZTE marked this conversation as resolved
@ -403,0 +407,4 @@
target,
};
pub fn createTexture(
Owner

Create a struct CreateTextureOptions, add fields for all possible options including sensible defaults where possible. createTexture should only take this struct as an argument (except for the self parameter).

Create a struct `CreateTextureOptions`, add fields for all possible options including sensible defaults where possible. `createTexture` should only take this struct as an argument (except for the `self` parameter).
LordMZTE marked this conversation as resolved
@ -403,0 +409,4 @@
pub fn createTexture(
self: *Sdl2Platform,
width: c_int,
Owner

Take a zenolith.layout.Size instead of 2 separate size params in CreateTextureOptions.

Take a `zenolith.layout.Size` instead of 2 separate size params in `CreateTextureOptions`.
LordMZTE marked this conversation as resolved
@ -403,0 +411,4 @@
self: *Sdl2Platform,
width: c_int,
height: c_int,
pixel_format: util.PixelFormat,
Owner

This should default to .static in the options struct.

This should default to `.static` in the options struct.
LordMZTE marked this conversation as resolved
@ -403,0 +430,4 @@
) orelse return error.CreateTexture;
errdefer c.SDL_DestroyTexture(texture);
if (c.SDL_SetTextureBlendMode(texture, c.SDL_BLENDMODE_BLEND) != 0) return error.CreateTexture;
Owner

This is use case-specific, take an optional parameter in the options struct and set this accordingly.

This is use case-specific, take an optional parameter in the options struct and set this accordingly.
LordMZTE marked this conversation as resolved
@ -22,2 +22,4 @@
return .{ .width = @intCast(w), .height = @intCast(h) };
}
pub fn updateTexture(self: *Sdl2Texture, comptime T: type, pixels: []const T, pitch: c_int) !void {
Owner

It makes no sense for this to be generic, the original just takes a void* due to crap API design. Take a []const u8 here. Also use u31 for pitch, negative pitch doesn't make sense.

It makes no sense for this to be generic, the original just takes a `void*` due to crap API design. Take a `[]const u8` here. Also use `u31` for `pitch`, negative pitch doesn't make sense.
LordMZTE marked this conversation as resolved
@ -23,1 +23,4 @@
}
pub fn updateTexture(self: *Sdl2Texture, comptime T: type, pixels: []const T, pitch: c_int) !void {
if (c.SDL_UpdateTexture(self.tex, null, pixels.ptr, pitch) != 0) return error.UpdateTexture;
Owner

Don't hard-code null here, take a ?zenolith.layout.Rectangle and use it here if provided.

Don't hard-code null here, take a `?zenolith.layout.Rectangle` and use it here if provided.
LordMZTE marked this conversation as resolved
src/util.zig Outdated
@ -413,2 +413,4 @@
};
}
pub const PixelFormat = enum {
Owner

Remove the unknown variant, turn this into an enum(c_int) and move it into Sdl2Texture.

Remove the `unknown` variant, turn this into an `enum(c_int)` and move it into `Sdl2Texture`.
LordMZTE marked this conversation as resolved
src/util.zig Outdated
@ -415,0 +458,4 @@
NV21,
};
pub fn toSdlPixelFormat(pixel_format: PixelFormat) c_int {
Owner

This function is redundant.

This function is redundant.
LordMZTE marked this conversation as resolved
coding-agent added 1 commit 2024-03-18 19:31:41 +01:00
Author
Contributor

Thanks for the review!

A decent first try knowing you, although there's a lot of improvements to be done.
Also change Sdl2Platform.createFont such that it uses createFont and Sdl2Font.addAtlasSprite such that it uses updateTexture. Thanks!

I'm thinking on the logic to implement this but I am not too sure how. Because to use this function I should have the platform already and call it as a method. I think a way around it would be to have the function receiving the renderer as an argument instead of self. Please let me know your thought on it.

Thanks for the review! > A decent first try knowing you, although there's a lot of improvements to be done. > Also change `Sdl2Platform.createFont` such that it uses `createFont` and `Sdl2Font.addAtlasSprite` such that it uses `updateTexture`. Thanks! I'm thinking on the logic to implement this but I am not too sure how. Because to use this function I should have the platform already and call it as a method. I think a way around it would be to have the function receiving the renderer as an argument instead of self. Please let me know your thought on it.
Owner

You should simply replace the *c.SDL_Texture in Sdl2Font with a Sdl2Texture which you can then invoke setPixels on.

You should simply replace the `*c.SDL_Texture` in `Sdl2Font` with a `Sdl2Texture` which you can then invoke `setPixels` on.
LordMZTE requested changes 2024-03-18 20:21:07 +01:00
LordMZTE left a comment
Owner

Weitermachen 👍

Weitermachen 👍
@ -402,1 +402,4 @@
}
const CreateTextureOptions = struct {
width: c_int,
height: c_int,
Owner

Take a zenolith.layout.Size instead of width and height.

Take a `zenolith.layout.Size` instead of width and height.
LordMZTE marked this conversation as resolved
@ -403,0 +405,4 @@
height: c_int,
pixel_format: Sdl2Texture.PixelFormat,
pixel_access: Sdl2Texture.PixelAccess = .static,
blend_mode: Sdl2Texture.TextureBlendMode = .none,
Owner

On second thought, let's remove this from createTexture and instead add a setBlendMode function to Sdl2Texture.

On second thought, let's remove this from createTexture and instead add a `setBlendMode` function to `Sdl2Texture`.
LordMZTE marked this conversation as resolved
@ -403,0 +418,4 @@
) orelse return error.CreateTexture;
errdefer c.SDL_DestroyTexture(texture);
if (c.SDL_SetTextureBlendMode(texture, options.blend_mode) != 0) return error.CreateTexture;
Owner

Remove this.

Remove this.
LordMZTE marked this conversation as resolved
@ -22,2 +80,4 @@
return .{ .width = @intCast(w), .height = @intCast(h) };
}
pub fn updateTexture(self: *Sdl2Texture, pixels: []const u8, pitch: u31, rect: ?zenolith.layout.Rectangle) !void {
Owner

Rename to setPixels.

Rename to `setPixels`.
LordMZTE marked this conversation as resolved
@ -23,1 +81,4 @@
}
pub fn updateTexture(self: *Sdl2Texture, pixels: []const u8, pitch: u31, rect: ?zenolith.layout.Rectangle) !void {
if (c.SDL_UpdateTexture(self.tex, rect, pixels.ptr, pitch) != 0) return error.UpdateTexture;
Owner

Add an std.debug.assert(pixels.len % pitch == 0); before this as a sanity check.

Also, you're missing an @intFromEnum here, this will almost certainly result in a compile error once this is referenced, which will happen once you make the font stuff use this :D

Add an `std.debug.assert(pixels.len % pitch == 0);` before this as a sanity check. Also, you're missing an `@intFromEnum` here, this will almost certainly result in a compile error once this is referenced, which will happen once you make the font stuff use this :D
LordMZTE marked this conversation as resolved
src/util.zig Outdated
@ -413,2 +413,4 @@
};
}
pub const PixelFormat = enum(c_int) {
Owner

Why is this here? We already have this in Sdl2Texture.

Why is this here? We already have this in `Sdl2Texture`.
LordMZTE marked this conversation as resolved
coding-agent added 1 commit 2024-03-18 22:20:51 +01:00
Author
Contributor

Vorfreude! (probably not using the word correctly but I think you get it)

Vorfreude! (probably not using the word correctly but I think you get it)
LordMZTE requested changes 2024-03-18 22:38:17 +01:00
src/Sdl2Font.zig Outdated
@ -31,3 +33,3 @@
pub fn deinit(self: *Sdl2Font) void {
c.SDL_DestroyTexture(self.atlas);
c.SDL_DestroyTexture(self.atlas.tex);
Owner

Add a deinit function to Sdl2Texture and call that instead.

Add a `deinit` function to `Sdl2Texture` and call that instead.
coding-agent marked this conversation as resolved
@ -403,0 +406,4 @@
pub fn createTexture(self: Sdl2Platform, options: CreateTextureOptions) !Texture {
const texture = c.SDL_CreateTexture(
self.renderer,
@intCast(@intFromEnum(options.pixel_format)),
Owner

intCast shouldn't be required here; if these params aren't c_ints, change the repr of the enum instead.

`intCast` shouldn't be required here; if these params aren't `c_int`s, change the repr of the enum instead.
coding-agent marked this conversation as resolved
@ -403,0 +411,4 @@
options.size.width,
options.size.height,
) orelse return error.CreateTexture;
errdefer c.SDL_DestroyTexture(texture);
Owner

This is redundant as no errors can be returned after this line.

This is redundant as no errors can be returned after this line.
coding-agent marked this conversation as resolved
@ -11,2 +12,4 @@
const Sdl2Texture = @This();
/// removed 32 formats as some of them are the same values
/// as 8888 formats
Owner

Replace the comment with this:

/// 32-bit integer formats have been removed, as they're assigned their `8888` equivalents
/// depending on system endianness. Directly use those instead.

(also understand what this means :D)

Replace the comment with this: ``` /// 32-bit integer formats have been removed, as they're assigned their `8888` equivalents /// depending on system endianness. Directly use those instead. ``` (also understand what this means :D)
Author
Contributor

I will read more about it...

I will read more about it...
coding-agent marked this conversation as resolved
@ -22,2 +79,4 @@
return .{ .width = @intCast(w), .height = @intCast(h) };
}
pub fn setPixels(self: *Sdl2Texture, pixels: []const u8, pitch: u31, rect: ?zenolith.layout.Rectangle) !void {
Owner

No need to use a pointer for self.

No need to use a pointer for `self`.
coding-agent marked this conversation as resolved
@ -24,0 +83,4 @@
std.debug.assert(pixels.len % pitch == 0);
const rec = if (rect) |r| util.toSdlRect(r) else null;
if (c.SDL_UpdateTexture(self.tex, @ptrCast(&rec), pixels.ptr, pitch) != 0) return error.UpdateTexture;
Owner

This is unsound, as this will incorrectly cast a pointer to an optional to an optional pointer in some cases. Do not throw around ptrCast like this. The & should go inside the if.

This is unsound, as this will incorrectly cast a pointer to an optional to an optional pointer in some cases. Do not throw around `ptrCast` like this. The `&` should go inside the `if`.
coding-agent marked this conversation as resolved
@ -24,0 +86,4 @@
if (c.SDL_UpdateTexture(self.tex, @ptrCast(&rec), pixels.ptr, pitch) != 0) return error.UpdateTexture;
}
pub fn setBlendMode(self: *Sdl2Texture, blend_mode: TextureBlendMode) !void {
Owner

No need to use a pointer for self.

No need to use a pointer for `self`.
coding-agent marked this conversation as resolved
coding-agent added 1 commit 2024-03-18 23:03:29 +01:00
LordMZTE requested changes 2024-03-18 23:07:19 +01:00
LordMZTE left a comment
Owner

One last thing!

One last thing!
@ -3,6 +3,7 @@ const std = @import("std");
const zenolith = @import("zenolith");
const c = @import("ffi.zig").c;
const util = @import("util.zig");
Owner

Redundant import

Redundant import
Author
Contributor

This import is actually being used here

This import is actually being used [here](https://git.mzte.de/zenolith/zenolith-sdl2/src/commit/9912cd5b6a3a8483e3b430b1f912a0f4bf71c475/src/Sdl2Texture.zig#L85)
Owner

My bad!

My bad!
LordMZTE marked this conversation as resolved
LordMZTE merged commit 563e01c2f2 into master 2024-03-18 23:16:49 +01:00
LordMZTE deleted branch feature/create-texture 2024-03-18 23:16:49 +01:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: zenolith/zenolith-sdl2#2
No description provided.