linguini1 commented on code in PR #3644:
URL: https://github.com/apache/nuttx-apps/pull/3644#discussion_r3596307601
##########
games/NXDoom/src/doom/r_bsp.c:
##########
@@ -78,7 +78,7 @@ line_t *linedef;
sector_t *frontsector;
sector_t *backsector;
-drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS];
+drawseg_t *drawsegs;
Review Comment:
Why is this change necessary?
##########
games/NXDoom/src/doom/r_main.c:
##########
@@ -685,6 +685,24 @@ fixed_t r_scale_from_global_angle(angle_t visangle)
void r_set_view_size(int blocks, int detail)
{
+ /* screenblocks is only ever meant to hold 3..11 (set that way by the
+ * options menu and by the config default of 9). The renderer's view
+ * geometry math divides by values derived from it - notably
+ * pspriteiscale = FRACUNIT * SCREENWIDTH / viewwidth in
+ * r_execute_set_view_size() - so a 0 or otherwise out-of-range value
+ * turns into a divide-by-zero hardware exception (EXCCAUSE=6), which on
+ * this flat-memory build takes the whole board down rather than just
+ * this task. Clamp defensively so a bad/missing config value degrades
+ * to the default screen size instead of a system crash.
+ */
+
+ if (blocks < 3 || blocks > 11)
+ {
+ printf("r_set_view_size: screenblocks=%d out of range, using 10\n",
+ blocks);
+ blocks = 10;
+ }
Review Comment:
Did you actually run into an error that caused this?
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -57,12 +57,12 @@ planefunction_t ceilingfunc;
/* Here comes the obnoxious "visplane". */
-visplane_t visplanes[CONFIG_GAMES_NXDOOM_MAXVISPLANES];
+visplane_t *visplanes;
Review Comment:
Ditto
##########
games/NXDoom/src/i_main.c:
##########
@@ -57,17 +57,28 @@ void d_doom_main(void);
int main(int argc, char **argv)
{
- /* save arguments */
+ /* save arguments
+ *
+ * +1 and an explicit NULL terminator: argv is conventionally
+ * NULL-terminated at argv[argc] (this is what the OS/exec path
+ * guarantees for the `argv` parameter above), and some of this
+ * codebase's own argument handling was written assuming that holds
+ * for myargv too - an under-sized allocation here leaves myargv[argc]
+ * pointing at whatever the allocator happens to return next, which
+ * only reads as "probably zero" by chance depending on heap layout.
+ */
myargc = argc;
- myargv = malloc(argc * sizeof(char *));
+ myargv = malloc((argc + 1) * sizeof(char *));
assert(myargv != NULL);
for (int i = 0; i < argc; i++)
Review Comment:
Did you actually run into an error with this?
##########
games/NXDoom/src/d_iwad.c:
##########
@@ -271,6 +271,18 @@ static void buld_iwad_dir_list(void)
add_iwad_dir(m_dir_name(myargv[0]));
+ /* Add the board's configured DOOM data directory. Kconfig documents
+ * CONFIG_GAMES_NXDOOM_PREFDIR as "Directory where DOOM WAD files are
+ * stored", but until now it was only used for the config/save file
+ * location -- nothing actually searched it for IWADs, forcing every
+ * launch to rely on the current directory or DOOMWADDIR/DOOMWADPATH
+ * being set by hand first.
+ */
+
+#ifdef CONFIG_GAMES_NXDOOM_PREFDIR
+ add_iwad_dir(CONFIG_GAMES_NXDOOM_PREFDIR);
+#endif
Review Comment:
This value should always be defined. No need for ifdef.
##########
games/NXDoom/Makefile:
##########
@@ -2,7 +2,7 @@ include $(APPDIR)/Make.defs
# Program options
-MODULE = $(CONFIG_GAMES_NXDOOM)
+MODULE = m
Review Comment:
Why do this?
##########
games/NXDoom/src/doom/statdump.c:
##########
@@ -39,7 +39,7 @@
* Pre-processor Definitions
****************************************************************************/
-#define MAX_CAPTURES 32
+#define MAX_CAPTURES 4
Review Comment:
Why change this? Can this be a Kconfig option?
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -195,7 +206,48 @@ static void r_make_spans(int x, int t1, int b1, int t2,
int b2)
void r_init_planes(void)
{
- /* Doh! */
+ /* These renderer scratch buffers are sized for a comfortable margin
+ * above vanilla DOOM's original limits and would blow the platform's
+ * internal DRAM budget as static arrays, so they're heap-allocated
+ * instead (comes out of the PSRAM-backed user heap on this target).
+ */
+
+ visplanes = malloc(sizeof(visplane_t) * CONFIG_GAMES_NXDOOM_MAXVISPLANES);
+ openings = malloc(sizeof(short) * MAXOPENINGS);
+ drawsegs = malloc(sizeof(drawseg_t) * CONFIG_GAMES_NXDOOM_MAXDRAWSEGS);
+ vissprites = malloc(sizeof(vissprite_t) *
+ CONFIG_GAMES_NXDOOM_MAXVISSPRITES);
+
+ if (visplanes == NULL || openings == NULL || drawsegs == NULL ||
+ vissprites == NULL)
+ {
+ i_error("r_init_planes: failed to allocate renderer buffers");
+ }
Review Comment:
I do not think we should be allocating these, at least not without a Kconfig
switch. I think the static allocation is better.
##########
games/NXDoom/src/i_video.c:
##########
@@ -262,27 +269,61 @@ static void blit_screen(void)
uint8_t p_idx;
void *fbptr;
- /* TODO: It would be best to do this more efficiently/with less memory.
- * It also would be good if we could handle the palette translation here
- * such that DOOM can be played on frame buffers with differing bit depths
- * and pixel formats.
- */
+ /* TODO: It would be best to do this more efficiently/with less memory. */
- fbptr = g_graphics_state.fbmem;
- for (unsigned y = 0; y < SCREENHEIGHT * g_graphics_state.scale; y++)
+ fbptr = g_graphics_state.fbmem +
+ g_graphics_state.yoffset * g_graphics_state.pinfo.stride +
+ g_graphics_state.xoffset *
+ (g_graphics_state.pinfo.bpp == 16 ? 2 : 4);
+
+ if (g_graphics_state.pinfo.bpp == 16)
Review Comment:
The only thing different about these loops is the pixel format. Put the `if`
statement inside the loop at the pixel conversion step.
Do you have a 16-bit device to test on?
##########
games/NXDoom/src/m_config.c:
##########
@@ -2098,7 +2098,9 @@ static void load_default_collection(default_collection_t
*collection)
while (!feof(f))
{
- if (fscanf(f, "%79s %99[^\n]\n", defname, strparm) != 2)
+ strparm[0] = '\0';
Review Comment:
Again why change this? Did you run into an error or was this determined by
an AI agent? Can you give a reproduction?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
