-
Notifications
You must be signed in to change notification settings - Fork 4
Add SDL3_gfx.pas #31
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
Add SDL3_gfx.pas #31
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @suve,
wow, great contribution! Thanks a lot.
I'd suggest some minor changes though. Please have a look into them and let me know what you think.
Best regards
Matthias
units/SDL3_gfx.pas
Outdated
|
||
{$I sdl.inc} | ||
|
||
Interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to use lower case for key words like Interface
, Implementation
and End
. Just a minor thing and nothing I would to refuse to merge; but that's usually the convention (in Pascal).
units/SDL3_framerate.inc
Outdated
SPDX-License-Identifier: Zlib | ||
} | ||
|
||
Const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion: Lower-case for Const
and Type
to be in accordance with wide spread Pascal conventions.
units/SDL3_gfxPrimitives.inc
Outdated
external GFX_LibName {$IFDEF DELPHI} {$IFDEF MACOS} name '_gfxPrimitivesSetFont' {$ENDIF} {$ENDIF}; | ||
procedure gfxPrimitivesSetFontRotation(rotation: cuint32); cdecl; | ||
external GFX_LibName {$IFDEF DELPHI} {$IFDEF MACOS} name '_gfxPrimitivesSetFontRotation' {$ENDIF} {$ENDIF}; | ||
function characterColor(renderer: PSDL_Renderer; x, y: cint16; c: cuint8; color: cuint32): Boolean; cdecl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument c
is of C type char
. We should translate it by cchar I guess (ctypes translates it to shortint
, hence cint8
). According to C documentation, char can be signed or unsigned if unspecified. https://en.wikipedia.org/wiki/C_data_types
[this applies for both character functions]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In C code, you'd often use char
for both "character" and "8-bit int". Since here it's pretty clear that c
is meant to be a character, I think it makes more sense to use AnsiChar
as the type (similarly to how we use PAnsiChar
for pointers to C-strings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convincing argument. Then let's go with AnsiChar
.
units/SDL3_gfxPrimitives.inc
Outdated
external GFX_LibName {$IFDEF DELPHI} {$IFDEF MACOS} name '_characterColor' {$ENDIF} {$ENDIF}; | ||
function characterRGBA(renderer: PSDL_Renderer; x, y: cint16; c, r, g, b, a: cuint8): Boolean; cdecl; | ||
external GFX_LibName {$IFDEF DELPHI} {$IFDEF MACOS} name '_characterRGBA' {$ENDIF} {$ENDIF}; | ||
function stringColor(renderer: PSDL_Renderer; x, y: cint16; const s: pcuint8; color: cuint32): Boolean; cdecl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument s
is a C char pointer, which we translate with PAnsiChar
according to our guidelines.
[this applies for both string functions]
units/SDL3_gfxPrimitives.inc
Outdated
SPDX-License-Identifier: Zlib | ||
} | ||
|
||
Const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I'd suggest lower-case for keyword Const
.
// | ||
|
||
// SDL_imageFilterAdd: D = saturation255(S1 + S2) | ||
function SDL_imageFilterAdd(Src1, Src2, Dest: pcuint8; length: cuint): cint; cdecl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following applies to all functions of the imageFilter inc file.
In accordance with ctypes unit, the following translation should be used
char -> cchar
char * -> pcchar
unsigned char -> cuchar
unsigned char * -> pcuchar
I know, you always picked the right sized types by hand. Unfortunately, to be as tight as possible at the original code and as we decided to use ctypes, we should use the translations as mentioned above, I guess. Apart from this, the files looks great.
SPDX-License-Identifier: Zlib | ||
} | ||
|
||
Const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Suggesting lower-case for keyword const
.
units/SDL3_gfxPrimitives_font.inc
Outdated
Const | ||
GFX_FONTDATAMAX = (8*256); | ||
|
||
gfxPrimitivesFontdata: Array[0..(GFX_FONTDATAMAX-1)] of Byte = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Byte
should be cuchar
.
As I consider these units (SDL3_gfx and SDL3_mixer) as rather important, I'd prefer to have them merged earlier than later. Do you mind, if we merge them soon/now and add my change requests as issues for later treatment? This would also mean, I could easily fix them myself without pushing commits to your branch. Best regards |
Great! Thanks! |
9ea96d3
into
PascalGameDevelopment:main
No description provided.