Map generator -verbose and -performance flags #2721
Conversation
WalkthroughThreads context.Context and a slog-based, flag-driven logger through the map-generator: new CLI flags and per-map contextual loggers; GenerateMap and many helpers now accept ctx. README documents CLI flags. No map-generation algorithms changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI
participant LoggerInit as Logger Init
participant Main as main (orchestrator)
participant Gen as GenerateMap(ctx)
participant Helpers as Processing Helpers
participant Stdout
User->>CLI: run tool with flags (--maps, --log-level, --verbose, --debug-*)
CLI->>LoggerInit: build LogFlags & NewGeneratorLogger(out, opts, flags)
LoggerInit-->>CLI: slog.Logger (handler installed, default)
CLI->>Main: spawn per-map goroutines
Main->>LoggerInit: create per-map slog.Logger & ctxWithLogger (map name tag)
Main->>Gen: call GenerateMap(ctxWithLogger, GeneratorArgs)
Gen->>Helpers: call helpers with ctx (processWater, packTerrain, createThumbnail, ...)
Helpers-->>Gen: return results / errors (use ctx-logger for messages)
Gen->>Stdout: emit logs via LoggerFromContext(ctx)
note right of Stdout: GeneratorLogger filters by level & tags,\nprefixes map name, and writes formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
map-generator/map_generator.go (1)
94-110: Rename parameter to avoid shadowing the type name.The parameter name
LogArgson line 97 shadows the type nameLogArgs, which can be confusing for readers. While Go allows this, it reduces code clarity.🔎 Suggested fix
- return func(LogArgs LogArgs, format string, v ...interface{}) { - if LogArgs.isVerbose && !args.Verbose { + return func(logArgs LogArgs, format string, v ...interface{}) { + if logArgs.isVerbose && !args.Verbose { return } - if LogArgs.isPerformance && !args.Performance { + if logArgs.isPerformance && !args.Performance { return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
map-generator/README.mdmap-generator/main.gomap-generator/map_generator.go
🔇 Additional comments (10)
map-generator/main.go (3)
16-17: LGTM! Flag declarations follow Go conventions.The package-level flag variables follow the same pattern as the existing
mapsFlagand use clear, descriptive names.
136-137: Correct flag usage with proper test map exclusion.The logic correctly passes the verbose flag to all maps while excluding test maps from performance warnings using
performanceFlag && !isTest. This aligns with the PR objectives.
253-255: LGTM! Idiomatic flag registration.Registering both
--verboseand-vto the same variable is the standard Go pattern for providing shorthand flags. The help descriptions are clear and concise.map-generator/README.md (1)
36-40: LGTM! Clear and complete documentation.The new "Command Line Flags" section clearly documents all available flags, including both the longhand and shorthand forms for the verbose option. The descriptions are concise and helpful.
map-generator/map_generator.go (6)
21-28: LGTM! Constants align with documented recommendations.The performance threshold constants match the README's guidance on map dimensions (2-3 million pixels) and provide clear, self-documenting values for the performance checks.
85-92: Clean logging abstraction.The
LogArgsstruct andLoggerfunction type provide a simple, pluggable logging system that supports conditional output based on verbose and performance flags. The design is straightforward and effective.
81-83: LGTM! Clean extension of GeneratorArgs.The new
VerboseandPerformanceboolean fields integrate cleanly into the existing struct without breaking changes.
150-155: Correct performance check implementation.The area calculation and range check correctly implement the performance warning requirement. The check is non-fatal and only runs when the performance flag is enabled, as specified in the PR objectives.
210-219: Comprehensive land tile count logging.The verbose logs provide the requested land tile counts at all scales (1x, 4x, 16x), and the performance warnings catch both edge cases (zero land tiles and excessive land tiles). This fulfills the PR objectives.
303-303: Consistent logger propagation throughout the codebase.The logger parameter has been systematically added to all relevant functions, with appropriate usage of verbose and performance flags at each call site. This creates a clean, consistent logging approach across the entire map generation workflow.
Also applies to: 341-341, 423-423, 527-527, 584-584, 623-623, 727-727
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
map-generator/map_generator.go (3)
94-110: Small nit: incomplete comment on line 96.The comment
// and handle checking against the performance and verbose flagsstarts with "and" which suggests it was meant to continue from somewhere. Consider making it a complete sentence.🔎 Suggested fix
// createLogger creates a logger instance that handles formatting and conditional output +// based on the performance and verbose flags. func createLogger(args GeneratorArgs) Logger { - // and handle checking against the performance and verbose flags return func(logArgs LogArgs, format string, v ...interface{}) {
149-155: Redundant check: the logger already handles this.The
createLoggerfunction already skips logs whenlogArgs.isPerformanceis true butargs.Performanceis false. The outerif args.Performancewrapper is not needed.🔎 Suggested fix
- // only perform this check if the performance flag is enabled - if args.Performance { - area := width * height - if area < minRecommendedPixelSize || area > maxRecommendedPixelSize { - logger(LogArgs{isPerformance: true}, "⚠️ Map area %d pixels is outside recommended range (%d - %d)", area, minRecommendedPixelSize, maxRecommendedPixelSize) - } + area := width * height + if area < minRecommendedPixelSize || area > maxRecommendedPixelSize { + logger(LogArgs{isPerformance: true}, "⚠️ Map area %d pixels is outside recommended range (%d - %d)", area, minRecommendedPixelSize, maxRecommendedPixelSize) }
727-737: Consider making binary data logging verbose-only.The
logBinaryAsBitsfunction logs raw binary data which is mainly useful for debugging. When running many maps, this output could be noisy. Consider gating it behind the verbose flag.🔎 Suggested fix
- logger(LogArgs{}, "Binary data (bits): %s", bits) + logger(LogArgs{isVerbose: true}, "Binary data (bits): %s", bits)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/map_generator.go
🔇 Additional comments (9)
map-generator/map_generator.go (9)
21-28: Looks good!Constants are clear, well-named, and easy to understand. Good use of separate const blocks for grouping related values.
81-92: Clean type design.The
Loggerfunction type andLogArgsstruct are simple and easy to follow. Using a function type instead of an interface keeps things lightweight.
206-219: Good use of logging levels.The land tile count logs are correctly marked as verbose, and the performance warnings use the performance flag. This matches the PR objectives well.
303-304: Clean update.Function signature updated to accept the logger. The log message is always shown which is appropriate for this processing step.
341-342: Looks good.
423-492: Good implementation of verbose logging for water body processing.The detailed removal coordinates (line 476) are correctly gated behind the verbose flag, while summary information remains always visible. This matches the PR objectives well.
527-573: Consistent with water body handling.Same pattern as
processWater: detailed per-island removal is verbose, summary is always shown. Good consistency.
584-617: Clean update to packTerrain.
623-624: Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
map-generator/map_generator.go (1)
728-738: Binary logging should be verbose-only.The
logBinaryAsBitsfunction logs with emptyLogArgs{}, meaning binary data is always logged regardless of verbose mode. This appears in the output for all three scales (1x, 4x, 16x) during every map generation.Binary representation of packed terrain is debugging information that should only appear when
-verboseis enabled, per the PR objective to preserve default behavior.🔎 Proposed fix
- logger(LogArgs{}, "Binary data (bits): %s", bits) + logger(LogArgs{isVerbose: true}, "Binary data (bits): %s", bits)
🧹 Nitpick comments (1)
map-generator/map_generator.go (1)
85-89: Consider exporting LogArgs fields for clarity.The
LogArgsstruct is exported but its fieldsisVerboseandisPerformanceare unexported. This prevents external packages from setting these fields directly and is an unconventional Go pattern. Since all usage appears internal to this package, consider either:
- Exporting the fields (rename to
IsVerbose,IsPerformance)- Or unexport the struct entirely (rename to
logArgs)The current design works but may confuse future maintainers expecting standard Go conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/map_generator.go (2)
85-114: LGTM: Well-designed logger abstraction.The closure-based logger cleanly captures the generator args and provides conditional output based on both message type and global flags. The name prefixing in verbose mode matches the PR requirements exactly.
Optional style notes:
- Lines 98, 102: "dont" → "don't" in comments
- Line 108: The nested
fmt.Sprintfcalls are clear but could be micro-optimized by building a single format stringThese are minor style points that don't affect functionality.
477-477: Optional: Consider defensive check for empty coordinates.Accessing
coords[0]is safe here becausegetArea()always includes the starting tile. However, the invariant is subtle—a defensive check or comment would help future maintainers:Optional defensive pattern
if waterBodies[w].size < minLakeSize { + if len(waterBodies[w].coords) == 0 { + continue // Safety: skip empty body + } logger(loggerArgs{isVerbose: true}, "Removing small lake at %d,%d (size %d)", waterBodies[w].coords[0].X, waterBodies[w].coords[0].Y, waterBodies[w].size)Also applies to: 564-564
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/map_generator.go
🔇 Additional comments (6)
map-generator/map_generator.go (6)
21-28: LGTM: Clear performance thresholds.The constants provide sensible boundaries for map area and tile counts, with clear naming that makes their purpose immediately obvious.
77-83: LGTM: Clean flag additions to GeneratorArgs.The new boolean fields integrate cleanly with the existing struct and directly map to the CLI flags described in the PR objectives.
153-156: LGTM: Proper area bounds checking.The performance check correctly validates map area against both minimum and maximum thresholds, using the
isPerformanceflag so warnings only appear when requested.
215-220: LGTM: Comprehensive land tile validation.The checks catch both edge cases (zero land tiles indicating potential data issues) and performance concerns (excessive tiles), with clear warnings that respect the
--performanceflag.
140-140: LGTM: Clean logger propagation throughout the call chain.The logger is properly threaded through all processing functions with consistent signatures. This enables centralized control over logging behavior while keeping the implementation clean and testable.
Also applies to: 188-196, 304-305, 342-343, 424-425, 490-491, 528-528, 585-585, 617-617, 624-625, 728-728
12-12: WebP library dependency is secure and current.Version 1.4.0 is the latest release and includes the security fix for CVE-2023-4863 (out-of-bounds/heap buffer issue in libwebp). No further action needed.
|
This is good to review. This is isolated from and does not include the changes from #2709 - but if the recommendations in the README change over there, the logical checks here should be updated (note for future me, really)... once they are both merged they can always get updated in sync. The only implementation difference from the issue description is using a closure proxy instead of a bunch of formatter calls. I think this will scale better for future flags that might want to enable additional logging, or help to sort some of the current logging if that is desired ( @coderabbitai really wants the binary stuff behind the verbose flag, but i kept it always logging so the default output wouldnt change) |
|
@Tidwell: Thanks for the heads-up! I'll review the changes now. Your implementation approach using a closure proxy makes sense for scalability—it gives you a clean, centralized point to control logging behavior and makes it straightforward to add new flag-driven logging options in the future without scattering conditional logic throughout the codebase. I appreciate you preserving the default output behavior by keeping the binary logging always on. That's the right call for this PR since it maintains backward compatibility and doesn't surprise existing users of the tool. Good note about coordinating with PR #2709 once both are merged—keeping those README recommendations and the logical checks in sync will be important. Let me review the implementation now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Tidwell
force-pushed
the
map-generator-verbose
branch
from
42fee8f to
60a193d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
map-generator/map_generator.go (1)
21-27: Consider consolidating constants into the existing block.These new performance threshold constants could be moved into the existing
constblock (lines 16-19) to keep all map-generation constants together.🔎 Suggested refactor
const ( minIslandSize = 30 minLakeSize = 200 + // the recommended max area pixel size for input images + minRecommendedPixelSize = 2000000 + maxRecommendedPixelSize = 3000000 + // the recommended max number of land tiles in the output bin at full size + maxRecommendedLandTileCount = 3000000 ) -const ( - // the recommended max area pixel size for input images - minRecommendedPixelSize = 2000000 - maxRecommendedPixelSize = 3000000 - // the recommended max number of land tiles in the output bin at full size - maxRecommendedLandTileCount = 3000000 -) -
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
map-generator/README.mdmap-generator/logger.gomap-generator/main.gomap-generator/map_generator.go
🚧 Files skipped from review as they are similar to previous changes (1)
- map-generator/README.md
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/map_generator.go (1)
map-generator/logger.go (3)
LoggerFromContext(189-194)PerformanceLogTag(26-26)RemovalLogTag(29-29)
🔇 Additional comments (1)
map-generator/map_generator.go (1)
122-124: The performance warning at lines 122-124 does not need to distinguish between test and production maps. Whilemain.gotracks anIsTestflag, there is no documented requirement to exclude test maps from performance warnings. The README only specifies that "Maps should be between 2 - 3 million pixels square (area)" without any test map exception. The current implementation is correct—performance recommendations apply to all maps uniformly.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
map-generator/map_generator.go (5)
21-27: Consolidate const blocks.Per past feedback and Go conventions, merge this const block with the existing one at lines 16-19 to keep all package-level constants together.
🔎 Proposed refactor
-const ( - minIslandSize = 30 - minLakeSize = 200 -) - const ( + minIslandSize = 30 + minLakeSize = 200 + // the recommended max area pixel size for input images minRecommendedPixelSize = 2000000 maxRecommendedPixelSize = 3000000 // the recommended max number of land tiles in the output bin at full size maxRecommendedLandTileCount = 3000000 )
119-119: Consider structured logging over fmt.Sprintf.Instead of embedding formatted strings, pass structured key-value pairs to the logger. This improves downstream parsing and filtering.
🔎 Proposed refactor
- logger.Info(fmt.Sprintf("Processing Map: %s, dimensions: %dx%d", args.Name, width, height)) + logger.Info("Processing Map", "name", args.Name, "width", width, "height", height)
177-186: Consider structured logging for tile counts and performance warnings.Similar to line 119, prefer structured key-value pairs over fmt.Sprintf for better parsing and filtering.
🔎 Proposed refactor
- logger.Debug(fmt.Sprintf("Land Tile Count (1x): %d", mapNumLandTiles)) - logger.Debug(fmt.Sprintf("Land Tile Count (4x): %d", numLandTiles4x)) - logger.Debug(fmt.Sprintf("Land Tile Count (16x): %d", numLandTiles16x)) + logger.Debug("Land Tile Count", "scale", "1x", "count", mapNumLandTiles) + logger.Debug("Land Tile Count", "scale", "4x", "count", numLandTiles4x) + logger.Debug("Land Tile Count", "scale", "16x", "count", numLandTiles16x) if mapNumLandTiles == 0 { return MapResult{}, fmt.Errorf("Map has 0 land tiles") } if mapNumLandTiles > maxRecommendedLandTileCount { - logger.Info(fmt.Sprintf("Map has more land tiles (%d) than recommended maximum (%d)", mapNumLandTiles, maxRecommendedLandTileCount), PerformanceLogTag) + logger.Info("Map has more land tiles than recommended maximum", "count", mapNumLandTiles, "max", maxRecommendedLandTileCount, PerformanceLogTag) }
439-461: Consider structured logging in processWater.For consistency with the earlier suggestion, replace fmt.Sprintf with structured key-value pairs. This applies to lines 439, 443, 446, 454, and 461.
🔎 Example refactor
- logger.Info(fmt.Sprintf("Identified ocean with %d water tiles", largestWaterBody.size)) + logger.Info("Identified ocean", "tiles", largestWaterBody.size) if removeSmall { - logger.Info("Searching for small water bodies for removal") + logger.Info("Searching for small water bodies for removal") // No change needed for w := 1; w < len(waterBodies); w++ { if waterBodies[w].size < minLakeSize { - logger.Debug(fmt.Sprintf("Removing small lake at %d,%d (size %d)", waterBodies[w].coords[0].X, waterBodies[w].coords[0].Y, waterBodies[w].size), RemovalLogTag) + logger.Debug("Removing small lake", "x", waterBodies[w].coords[0].X, "y", waterBodies[w].coords[0].Y, "size", waterBodies[w].size, RemovalLogTag) smallLakes++ for _, coord := range waterBodies[w].coords { terrain[coord.X][coord.Y].Type = Land terrain[coord.X][coord.Y].Magnitude = 0 } } } - logger.Info(fmt.Sprintf("Identified and removed %d bodies of water smaller than %d tiles", smallLakes, minLakeSize)) + logger.Info("Identified and removed small bodies of water", "count", smallLakes, "minSize", minLakeSize) }
533-542: Consider structured logging in removeSmallIslands.Apply the same structured logging pattern for consistency with other functions.
🔎 Proposed refactor
- logger.Debug(fmt.Sprintf("Removing small island at %d,%d (size %d)", body.coords[0].X, body.coords[0].Y, body.size), RemovalLogTag) + logger.Debug("Removing small island", "x", body.coords[0].X, "y", body.coords[0].Y, "size", body.size, RemovalLogTag) smallIslands++ for _, coord := range body.coords { terrain[coord.X][coord.Y].Type = Water terrain[coord.X][coord.Y].Magnitude = 0 } } } - logger.Info(fmt.Sprintf("Identified and removed %d islands smaller than %d tiles", smallIslands, minIslandSize)) + logger.Info("Identified and removed small islands", "count", smallIslands, "minSize", minIslandSize)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
map-generator/logger.gomap-generator/main.gomap-generator/map_generator.go
🚧 Files skipped from review as they are similar to previous changes (2)
- map-generator/main.go
- map-generator/logger.go
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/map_generator.go (1)
map-generator/logger.go (3)
LoggerFromContext(203-208)PerformanceLogTag(26-26)RemovalLogTag(29-29)
🔇 Additional comments (1)
map-generator/map_generator.go (1)
105-106: LGTM: Context propagation is clean and idiomatic.The context threading through GenerateMap and all helper functions follows Go best practices. Logger retrieval via LoggerFromContext(ctx) is consistent throughout, and the fallback to slog.Default() (per logger.go) ensures safe operation even if context lacks a logger.
Also applies to: 154-163, 270-272, 309-311, 392-394, 496-497, 553-553, 592-594, 697-698
|
Refactored to isolate logging behavior and use the standard go |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/logger.go (2)
60-61: Consider writing the invalid log level warning to stderr.When a user provides an invalid value, writing to
os.Stderrmakes it clearer this is a warning rather than normal output. Helps if stdout is piped elsewhere.Suggested fix
- fmt.Printf("invalid log level: %s, defaulting to info\n", logFlags.logLevel) + fmt.Fprintf(os.Stderr, "invalid log level: %s, defaulting to info\n", logFlags.logLevel)And add
"os"to imports.
178-183: Slice aliasing may cause issues if handlers are derived concurrently.When copying the handler,
newHandler.attrsshares the backing array withh.attrs. If there is spare capacity,appendwon't allocate, and concurrentWithAttrscalls could step on each other.Force a fresh slice to avoid this:
Suggested fix
func (h *GeneratorLogger) WithAttrs(attrs []slog.Attr) slog.Handler { newHandler := *h - newHandler.attrs = append(newHandler.attrs, attrs...) + newHandler.attrs = make([]slog.Attr, len(h.attrs), len(h.attrs)+len(attrs)) + copy(newHandler.attrs, h.attrs) + newHandler.attrs = append(newHandler.attrs, attrs...) return &newHandler }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/logger.go
🔇 Additional comments (4)
map-generator/logger.go (4)
77-96: Clean constructor with good defensive coding.The nil check for
opts.Levelat lines 92-94 prevents potential nil pointer issues. Sharing the mutex pointer across derived handlers is the right approach for thread-safe writes.
106-176: Handle method looks correct; past review issues resolved.The operator precedence fix on line 156 with proper parentheses is in place. The group prefix is now written at lines 166-168. Filter logic for performance/removal flags and test maps is clear and works as intended.
185-197: WithGroup implementation is correct.The empty name guard at line 188-190 prevents unnecessary copies. String concatenation for the prefix is safe since Go strings are immutable.
199-213: Context utilities follow standard Go patterns.Using an unexported empty struct as the context key avoids collisions. Falling back to
slog.Default()when no logger is found is a sensible default.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/map_generator.go (2)
174-183: Good validation; enhance error message with map name.The zero land tile check now returns an error, which correctly addresses the previous review comment. The debug logs and performance warning provide useful observability.
Consider including the map name in the error message for easier debugging when processing multiple maps:
🔎 Suggested improvement
if mapNumLandTiles == 0 { - return MapResult{}, fmt.Errorf("Map has 0 land tiles") + return MapResult{}, fmt.Errorf("map %q has 0 land tiles", args.Name) }
550-584: Context propagation complete; consider logBinaryAsBits verbosity.The context threading through
packTerrain,createMapThumbnail, andlogBinaryAsBitsis consistent.The call to
logBinaryAsBitsat line 582 always logs binary representation of packed data. This seems like debugging detail that might clutter logs during normal operation, especially when processing multiple maps. Consider making this conditional (e.g., only at ALL log level) or removing if it's not needed for operational visibility.Also applies to: 589-616, 694-705
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/map_generator.go
🔇 Additional comments (6)
map-generator/map_generator.go (6)
15-24: Good documentation of constraints.The updated comments and new recommended size constants improve code clarity. The values provide clear boundaries for performance validation.
267-301: Context integration is clean.The function correctly accepts context and uses it for logging. The logic remains focused on shoreline identification.
306-350: Logging integration maintains algorithm clarity.Context and logging added without complicating the BFS distance calculation logic.
389-460: Well-structured logging for water body processing.The logging provides good visibility into water body identification, removal, and processing. The removal log at line 443 includes helpful details (coordinates, size) while using the appropriate tag for filtering.
493-540: Consistent logging pattern for island removal.Matches the water body removal pattern effectively. The debug log provides details while the info log gives a summary.
102-121: Context threading and logger handling are correctly implemented.The context parameter and logger retrieval follow Go patterns well. LoggerFromContext (map-generator/logger.go:203-208) safely handles missing loggers by returning
slog.Default()as a graceful fallback. The type assertion with theokcheck prevents nil returns and panics. The area validation logic correctly identifies maps outside the recommended range.No action needed—the logger availability pattern is robust throughout the file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
map-generator/map_generator.go (2)
116-116: Consider using structured logging attributes.The code uses
fmt.Sprintfto build log messages. While functional, slog supports structured key-value pairs that are more machine-parseable and don't require string formatting:logger.Info("Processing Map", "name", args.Name, "width", width, "height", height) area := width * height if area < minRecommendedPixelSize || area > maxRecommendedPixelSize { logger.Debug("Map area outside recommended range", "area", area, "min", minRecommendedPixelSize, "max", maxRecommendedPixelSize, PerformanceLogTag) }This pattern separates data from message formatting and enables better log querying.
Also applies to: 118-121
102-206: Consider adding context cancellation checks for long-running operations.While context is properly propagated, the code doesn't check for cancellation (
ctx.Err()orctx.Done()). For large maps, generation can take considerable time, and supporting cancellation would improve user experience:// Example in pixel processing loop (lines 130-149) for x := 0; x < width; x++ { if x%100 == 0 { // Check periodically, not every iteration if err := ctx.Err(); err != nil { return MapResult{}, fmt.Errorf("map generation cancelled: %w", err) } } for y := 0; y < height; y++ { // ... existing pixel processing } }Similar checks could be added in BFS loops within
getAreaandprocessDistToLand.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/map_generator.go
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/map_generator.go (1)
map-generator/logger.go (3)
LoggerFromContext(203-208)PerformanceLogTag(26-26)RemovalLogTag(29-29)
🔇 Additional comments (12)
map-generator/map_generator.go (12)
5-5: LGTM! Clean context integration and well-documented constants.The
contextimport and new performance threshold constants are well-chosen. The recommended ranges (2-3M pixels, max 3M land tiles) provide sensible guardrails for map generation performance.Also applies to: 15-24
102-103: LGTM! Idiomatic context propagation pattern.Accepting
context.Contextas the first parameter and extracting the logger viaLoggerFromContext(ctx)follows Go best practices. This enables per-map contextual logging as intended by the PR objectives.
151-152: LGTM! Consistent context propagation.All helper functions correctly receive the context parameter, maintaining a consistent pattern throughout the map generation pipeline.
Also applies to: 155-155, 158-158, 160-160
170-176: LGTM! Useful land tile count logging.The debug logs for land tile counts at each scale (1x, 4x, 16x) provide valuable diagnostics when running with verbose flags. The context-aware
packTerraincalls maintain consistency.
178-183: LGTM! Critical safeguards properly implemented.The zero land tiles check now returns an error, correctly addressing the past review comment. The performance warning for excessive land tiles provides helpful diagnostics without blocking generation. Both checks are appropriate safeguards.
267-269: LGTM! Context-aware shoreline processing.The function correctly accepts context and uses the extracted logger for informational output.
306-308: LGTM! Context-aware distance calculation.The function signature and logging follow the established pattern correctly.
389-391: LGTM! Comprehensive water body processing with granular logging.The function properly propagates context, uses appropriate log levels (Info for summaries, Debug with
RemovalLogTagfor individual removals), and passes context to nested calls. The logging granularity aligns well with the PR objectives for verbose and removal logging flags.Also applies to: 436-436, 440-443, 451-451, 455-459
493-494: LGTM! Consistent island removal logging.The function follows the same logging pattern as
processWater, using Debug level withRemovalLogTagfor individual removals and Info for summaries. Context handling is correct.Also applies to: 530-530, 539-539
550-550: LGTM! Pack terrain with context-aware logging.The function signature correctly accepts context and propagates it to
logBinaryAsBits. The addition ofnumLandTilesas a return value supports the new validation checks inGenerateMap.Also applies to: 582-582
589-591: LGTM! Context-aware thumbnail creation.The function correctly integrates context and logging following the established pattern.
694-695: LGTM! Binary logging updated for context.The debug helper function correctly accepts context and uses the extracted logger for binary output visualization.
Also applies to: 704-704
iiamlewis
added
the
Maps
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
map-generator/main.go
🧰 Additional context used
🧬 Code graph analysis (1)
map-generator/main.go (2)
map-generator/logger.go (4)
LogFlags(15-20)ContextWithLogger(211-213)NewGeneratorLogger(79-96)DetermineLogLevel(33-65)map-generator/map_generator.go (2)
GenerateMap(102-206)GeneratorArgs(73-77)
🔇 Additional comments (6)
map-generator/main.go (6)
4-4: LGTM! Appropriate imports for context propagation and structured logging.The addition of
contextandlog/slogpackages aligns with the PR objectives to implement context-aware, structured logging using Go's slog package.Also applies to: 9-9
70-74: LGTM! Standard Go flag pattern with clear documentation.The package-level flag variables follow Go conventions for flag parsing and are well-documented.
105-105: LGTM! Proper context propagation following Go idioms.The context parameter is correctly added as the first parameter to
processMapand properly threaded through toGenerateMap.Also applies to: 136-136
233-235: LGTM! Proper per-map logger and context wiring.Each goroutine correctly creates a per-map logger derived from the default logger with map-specific attributes, wraps it in a context, and passes it through to
processMap. This enables map-specific logging as intended.
258-264: LGTM! Flag definitions match PR objectives and follow Go conventions.The command-line flags are properly defined with clear help text, and the
-vshorthand correctly aliases-verboseusing the standard Go flag pattern.
266-274: LGTM! Proper slog logger initialization with custom handler.The logger is correctly initialized with the custom
GeneratorLoggerhandler, appropriate options, and log level determined from flags. Setting it as the default logger enables per-map loggers to derive from it.
| var MapLogTag = slog.String("map", mapItem.Name) | ||
| var TestLogTag = slog.Bool("isTest", mapItem.IsTest) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use lowercase names for local variables.
MapLogTag and TestLogTag follow Go's exported (public) naming convention but are local variables within the goroutine. In Go, capitalized names are reserved for exported identifiers. Use lowercase names for local scope.
🔎 Proposed fix
- var MapLogTag = slog.String("map", mapItem.Name)
- var TestLogTag = slog.Bool("isTest", mapItem.IsTest)
- logger := slog.Default().With(MapLogTag).With(TestLogTag)
+ mapLogTag := slog.String("map", mapItem.Name)
+ testLogTag := slog.Bool("isTest", mapItem.IsTest)
+ logger := slog.Default().With(mapLogTag).With(testLogTag)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var MapLogTag = slog.String("map", mapItem.Name) | |
| var TestLogTag = slog.Bool("isTest", mapItem.IsTest) | |
| mapLogTag := slog.String("map", mapItem.Name) | |
| testLogTag := slog.Bool("isTest", mapItem.IsTest) | |
| logger := slog.Default().With(mapLogTag).With(testLogTag) |
🤖 Prompt for AI Agents
In map-generator/main.go around lines 231-232, the local variables MapLogTag and
TestLogTag are capitalized (exported-style); rename them to lowercase (e.g.,
mapLogTag and testLogTag) and update all subsequent uses in the goroutine to the
new names so they remain local, non-exported identifiers and follow Go naming
conventions.
3 participants
Resolves #2718
Description:
Adds go-style error log levels, with an additional ALL log level.
In addition
-verbose(-v),-log-level,-log-removal, and-log-performanceflags to map generatorgo run .without the new flagsDefault run (matches existing)
go run . >> output.txt 2>&1output.txt
Default run w/
-verbose(log level DEBUG)go run . -v >> output.txt 2>&1output.txt
Default run w/
-log-performancego run . -log-performance >> output.txt 2>&1output.txt
Run of just africa w/ all new logging enabled
go run . -maps=africa -log-level=all >> output.txt 2>&1output.txt
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tidwell