Add SDL3_gfx.pas#31
Add SDL3_gfx.pas#31Free-Pascal-meets-SDL-Website merged 5 commits intoPascalGameDevelopment:mainfrom
Conversation
Free-Pascal-meets-SDL-Website
left a comment
Free-Pascal-meets-SDL-Website
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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! |
Free-Pascal-meets-SDL-Website
merged commit 9ea96d3
into
PascalGameDevelopment:main
2 participants
No description provided.