Feature/shape terrain - #65
Conversation
WalkthroughTwo new alteration spells, "Raise Wall" and "Dig," are introduced with their spell effects and JSON definitions. The spell system now uses string-based effect types instead of enums. New enum members for spell contexts and effects are added. Helpers for tile transformation and mining difficulty are created. Various nullability and code clarity improvements are applied across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant SpellSystem
participant TileHelpers
participant TileFactory
participant Map
Player->>SpellSystem: Cast "Raise Wall" or "Dig" spell
SpellSystem->>TileHelpers: ChangeTileEffect(target, caster, modifier, newType)
TileHelpers->>Map: Get tile at target location
TileHelpers->>PhysicsSystem: GetMiningDificulty(material)
alt Tile change successful
TileHelpers->>TileFactory: ChangeTileType(tile, newType)
TileFactory->>tile: Update appearance and properties
else Tile change failed
TileHelpers->>SpellSystem: Log error message
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (1)
src/Diviner/Windows/LookWindow.cs (1)
19-31: Inconsistent printing behavior detected in the entity constructor!The entity constructor builds a detailed description in
descbut only printsentity.Descriptionto the console, ignoring all the additional information gathered (descriptor, status, ID, position). This seems like unfinished enchantment work.Complete the spell by printing the full description:
- lookConsole.Cursor.Print(entity.Description); + lookConsole.Cursor.Print(desc.ToString());Or if you prefer to keep the original behavior, remove the unused
descbuilding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (33)
src/Arquimedes/Data/Spells/spells_alteration.json(1 hunks)src/Arquimedes/Enumerators/SpellContext.cs(1 hunks)src/Arquimedes/Enumerators/SpellEffectType.cs(2 hunks)src/Arquimedes/Enumerators/TargetState.cs(1 hunks)src/Arquimedes/Utils/FileUtils.cs(2 hunks)src/Diviner/KeyboardHandle.cs(3 hunks)src/Diviner/Windows/LookWindow.cs(3 hunks)src/MagusEngine/Bus/MapBus/ChangeControlledEntitiy.cs(0 hunks)src/MagusEngine/Core/Entities/Base/Anatomy.cs(2 hunks)src/MagusEngine/Core/Entities/Player.cs(3 hunks)src/MagusEngine/Core/Entities/Target.cs(13 hunks)src/MagusEngine/Core/Magic/Effects/DamageEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/DigEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/HasteEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/KnockbackEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/LightEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/MEssionEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/MageSightEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/PermEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/RaiseWallEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/SeverEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Effects/TeleportEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Interfaces/ISpellEffect.cs(1 hunks)src/MagusEngine/Core/Magic/Magic.cs(1 hunks)src/MagusEngine/Core/Magic/SpellEffectBase.cs(1 hunks)src/MagusEngine/Core/MapStuff/MagiMap.cs(17 hunks)src/MagusEngine/Core/MapStuff/Tile.cs(5 hunks)src/MagusEngine/Core/MapStuff/TileHelpers.cs(1 hunks)src/MagusEngine/MagusEngine.csproj(1 hunks)src/MagusEngine/Serialization/EntitySerialization/SpellTemplate.cs(1 hunks)src/MagusEngine/Services/Factory/SpellEffectFactory.cs(1 hunks)src/MagusEngine/Services/Factory/TileFactory.cs(1 hunks)src/MagusEngine/Systems/Physics/PhysicsSystem.cs(2 hunks)
💤 Files with no reviewable changes (1)
- src/MagusEngine/Bus/MapBus/ChangeControlledEntitiy.cs
🧰 Additional context used
🪛 GitHub Actions: Build and Test
src/MagusEngine/MagusEngine.csproj
[warning] 1-1: Numerous CS8600, CS8602, CS8604, CS8618, CS8625, CS8629, CS8765, CS8777, CS0169, and SYSLIB0051 warnings related to nullability, unused fields, and obsolete API usage.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: SonarScan
🔇 Additional comments (47)
src/Arquimedes/Enumerators/TargetState.cs (1)
5-5: By the ancient scrolls, a new targeting state emerges!The addition of
IdleModeto the targeting incantations appears most wise, dear mage! This new state shall provide clarity when our mystical targeting systems return to their restful state.However, placing this new enchantment as the first member of the enum may affect any arcane calculations that rely upon the numeric values of these states. Ensure thy spellwork elsewhere accounts for this reordering of the mystical constants!
Let me divine whether this enum ordering change affects other parts of the magical realm:
#!/bin/bash # Description: Search for potential integer usage of TargetState enum values # Expected: Find any code that might be affected by the enum reordering echo "=== Searching for TargetState integer conversions ===" rg -A 3 -B 3 "TargetState.*int|int.*TargetState|\(int\).*TargetState|TargetState.*\(int\)" echo "=== Searching for switch statements on TargetState ===" rg -A 10 "switch.*TargetState|switch.*targetState" echo "=== Searching for TargetState array indexing ===" rg -A 3 -B 3 "\[.*TargetState|\[.*targetState"src/MagusEngine/MagusEngine.csproj (1)
10-10: The mystical tomes of GoRogue have been updated, but dark omens appear!The upgrade from beta09 to beta10 of the GoRogue grimoire brings new powers, yet the crystal ball reveals troubling warnings in the build incantations! The pipeline speaks of numerous nullability curses (CS8600, CS8602, CS8604, CS8618, CS8625, CS8629, CS8765, CS8777) that may have awakened with this update.
Verily, ensure this version update aligns with the changes to MagiMap's inheritance mentioned in the ancient scrolls (the summary).
Allow me to consult the ethereal archives to verify this update brings no ill fortune:
What are the breaking changes and new features in GoRogue 3.0.0-beta10 compared to 3.0.0-beta09?src/MagusEngine/Core/Magic/Effects/KnockbackEffect.cs (1)
18-18: The mystical force of knockback transforms its essence from rigid enum to flowing string!This transmutation from the solid form of
EffectType.KNOCKBACKto the more etherealSpellEffectType.KNOCKBACK.ToString()aligns perfectly with the grand refactoring ritual described in the ancient texts! The spell effect now speaks in the common tongue of strings, allowing for easier divination through serialization and more flexible enchantment extensions.The invocation appears correctly wrought, maintaining the same magical essence while adopting the new paradigm.
src/MagusEngine/Core/Magic/Effects/HasteEffect.cs (1)
25-25: The winds of haste now speak in the universal tongue of strings!Behold! Another successful transmutation in our grand magical refactoring ritual! The
HasteEffectnow properly converts its mystical essence from the rigidEffectType.HASTEto the more flexibleSpellEffectType.HASTE.ToString(), following the same arcane pattern as its sibling effects.This change maintains perfect harmony with the system-wide transformation from strongly-typed enum mysticism to string-based enchantment identification. The spell of swiftness shall now flow more easily through serialization portals and extensibility gateways!
src/MagusEngine/Core/Magic/Effects/MageSightEffect.cs (1)
23-23: The Sight beyond sight is granted with proper form!This enchantment follows the same mystical pattern as the other spell effects in this grand transformation. The string-based effect typing shall serve the realm well.
src/Arquimedes/Enumerators/SpellEffectType.cs (2)
3-3: A most wise renaming of the mystical enumerations!Changing from
EffectTypetoSpellEffectTypebrings clarity to the arcane arts, making it crystal clear these enumerated powers belong to the realm of spell casting.
16-17: Behold! New powers of terrain manipulation are inscribed in the tome!The addition of
RAISEWALLandDIGspell effects opens new chapters in the art of shaping the very earth itself. These mystical forces shall grant wizards dominion over stone and soil!src/MagusEngine/Core/Entities/Player.cs (3)
25-25: The apprentice grows in power most magnificently!Raising the magic shaping ability to 65 grants the test wizard greater mastery over the mystical arts, necessary for wielding the new terrain-altering enchantments.
51-52: New spells of earth and stone are granted to the worthy!The addition of "dig" and "raise_wall" spells at level 2 brings forth the power to reshape the very foundations of the world. These incantations shall serve the test wizard well in mastering the terrain.
64-66: The grimoire expands with earthen magics!Adding the new terrain spells to the known spell list ensures our test wizard can practice the ancient arts of stone shaping and earth moving.
src/MagusEngine/Core/Magic/Effects/DamageEffect.cs (1)
23-23: The forces of destruction follow the new mystical pattern!The damage effect correctly adopts the string-based effect type system. However, I sense a potential disturbance in the magical flow - the
JsonConstructor(lines 27-43) doesn't set theEffectTypeproperty while the parameterless constructor does.Pray tell, is this intentional? Ensure that spell effects deserialized from JSON maintain their proper magical signatures.
#!/bin/bash # Description: Search for other JsonConstructor patterns in spell effects to verify consistency # Expected: Find if other spell effects set EffectType in JsonConstructor or rely on parameterless constructor echo "Searching for JsonConstructor patterns in spell effects..." rg -A 10 -B 2 "JsonConstructor" --type cs src/MagusEngine/Core/Magic/Effects/ echo -e "\nSearching for EffectType assignments in constructors..." rg -A 3 -B 3 "EffectType.*=" --type cs src/MagusEngine/Core/Magic/Effects/src/MagusEngine/Serialization/EntitySerialization/SpellTemplate.cs (1)
23-23: By the ancient tomes, this spell parsing enchantment looks most excellent!The transformation from the old
EffectTypetoSpellEffectTypeenum is woven correctly into the deserialization ritual. The spell effects shall be properly conjured from their JSON grimoire form.src/MagusEngine/Core/Magic/SpellEffectBase.cs (1)
16-16: ```shell
#!/bin/bash
set -eLocate SpellEffectBase.cs and display its contents for context
FILE=$(fd SpellEffectBase.cs | head -n1)
echo "File: $FILE"
sed -n '1,200p' "$FILE"</details> <details> <summary>src/Arquimedes/Enumerators/SpellContext.cs (1)</summary> `18-20`: **Magnificent additions to our arcane lexicon!** These new spell contexts - `Terraforming`, `Dig`, and `BlocksSight` - shall serve well for the earth-shaping magics being woven into our realm. The naming is clear and follows the established mystical conventions. </details> <details> <summary>src/MagusEngine/Core/Magic/Magic.cs (1)</summary> `24-24`: By the flicker of enchanted quills, let us summon deeper insight into the AddToSpellList incantation: ```shell #!/bin/bash rg -n "AddToSpellList" -C3 src/MagusEngine/Core/Magic/Magic.cs rg -n "KnowEffects\\.Contains" -C3 src/MagusEngine/Core/Magic/Magic.cssrc/MagusEngine/Core/Magic/Interfaces/ISpellEffect.cs (1)
20-20: By my beard and staff! This mystical transformation weakens our spell-binding magic!Changing from the strongly-typed
EffectTypeenum to a mere string removes the arcane protections of compile-time type checking. While I sense this is part of a grand transmutation across the realm, we must ensure our spell effects remain consistent and true!Let me divine the impact of this change across our magical domains:
#!/bin/bash # Description: Verify all spell effects use consistent string values # Expected: All effect types should use .ToString() pattern from SpellEffectType enum echo "=== Searching for EffectType assignments in spell effects ===" rg -A 2 -B 2 "EffectType\s*=" --type cs echo "=== Checking for SpellEffectType enum usage ===" rg "SpellEffectType\." --type cs echo "=== Looking for potential inconsistencies ===" rg -i "effecttype.*=" --type cs | grep -v "ToString()"src/MagusEngine/Core/Magic/Effects/SeverEffect.cs (1)
22-22: Excellent transmutation, young sorcerer!Your spell effect correctly follows the new incantation pattern, converting the mystical
SpellEffectType.SEVERto its string essence. The blade magic shall continue to serve us well in this new form!src/MagusEngine/Core/Entities/Base/Anatomy.cs (2)
99-101: Wisely woven protection spells for our anatomical divinations!These early return enchantments prevent us from casting
Existsupon empty collections - a most prudent ward against potential mystical mishaps! The logic remains sound: no organs means no sight, as clear as crystal.
110-112: Another excellent ward against the void!Your protective incantation ensures we don't seek a torso where no limbs exist. Most logical indeed - even the most powerful necromancer cannot conjure an upper body from naught!
src/MagusEngine/Services/Factory/TileFactory.cs (1)
131-141: Magnificent transmutation magic for reshaping the very foundations of our realm!This extension method is a masterfully crafted spell for changing tile essences! It properly updates all mystical properties - appearance, walkability, transparency, and name - ensuring complete transformation. The call to
UpdateLastSeenAppearence()shows wisdom in maintaining the visual consistency of our magical world.Let me ensure the
UpdateLastSeenAppearence()method exists in the Tile class:#!/bin/bash # Description: Verify UpdateLastSeenAppearence method exists on Tile class # Expected: Method should be found in Tile class definition echo "=== Searching for UpdateLastSeenAppearence method ===" rg -A 5 "UpdateLastSeenAppearence" --type cs echo "=== Checking Tile class for this method ===" ast-grep --pattern 'class Tile { $$$ UpdateLastSeenAppearence($$$) { $$$ } $$$ }'src/MagusEngine/Core/Magic/Effects/RaiseWallEffect.cs (1)
9-21: By the ancient scrolls, this spell implementation is most excellently crafted!The
RaiseWallEffectfollows the mystical patterns established by the arcane architecture. The delegation toTileHelpers.ChangeTileEffectmaintains proper separation of concerns, and the error messaging through the ethereal message bus ensures wizards are informed when their earth-shaping fails.src/MagusEngine/Core/Magic/Effects/DigEffect.cs (1)
9-21: Ah, the mystical arts of excavation! This incantation mirrors its wall-raising sibling most admirably!The
DigEffectspell follows the same arcane patterns as its earthen counterpart, maintaining consistency in the magical framework. The transmutation from solid stone to open passage is handled with the same wisdom and error-catching enchantments.src/Arquimedes/Utils/FileUtils.cs (2)
52-66: The formatting of this parallel enchantment has been made more readable to mortal eyes!The restructuring of the parallel processing incantation with proper brace placement enhances the clarity of the magical operations, though the essence of the spell remains unchanged.
21-21: Ah, the previous incantation stumbled over its own toes—let us cast a simpler find-ritual to unearth all hidden JSON spell scrolls in the vault:#!/bin/bash echo "🔍 Summoning the find ritual for JSON spell scrolls in subdirectories:" find src/Arquimedes/Data/Spells -type f -name "*.json"This shall reveal any
.jsonfiles nestling in deeper chambers.src/Arquimedes/Data/Spells/spells_alteration.json (1)
1-42: Behold! These terrestrial transmutation spells are woven with great skill!The spell definitions for both earth-shaping incantations are properly balanced and consistently structured. The magical parameters appear harmonious - both requiring level 2 mastery, 7 mana points, and 4 units of range. The contextual classifications are most wise: wall-raising blocks sight while digging creates passages.
One mystical query arises: the "IgnoresWall" property is set to
truefor both spells. Verify this is the intended behavior - should a wall-raising spell truly ignore existing walls, or should it perhaps fail when targeting an occupied space?src/MagusEngine/Core/MapStuff/Tile.cs (2)
185-188: An elegant method to preserve the visual essence of mystical tiles!The
UpdateLastSeenAppearence()method centralizes the cloning ritual, removing duplication from the constructors. This encapsulation ensures consistent behavior when refreshing the tile's remembered visage.
46-47: The constructor formatting has been blessed with proper brace alignment!The restructuring of constructor braces enhances readability while the calls to
UpdateLastSeenAppearence()maintain consistent appearance tracking across all tile creation paths.Also applies to: 61-61, 96-96
src/Diviner/KeyboardHandle.cs (2)
352-356: A mystical F2 key binding appears!The forced FOV recalculation debug feature looks properly implemented. The incantation calls the map's
ForceFovCalculation()method cleanly.
358-363: The K key now holds the power of transparency!This debug feature allows toggling tile transparency, which is most useful for testing sight mechanics. The null-conditional operator on
_targetCursor?.TileInTarget()is wisely used.src/MagusEngine/Services/Factory/SpellEffectFactory.cs (2)
22-23: New terraforming spells have been inscribed in the factory!The registration of "RAISEWALL" and "DIG" spell effects follows the established pattern perfectly. These new mystical powers will serve the wizards well.
26-26: The spell effect type transformation is complete!The parameter type changes from
EffectTypetoSpellEffectTypeare consistent and properly applied across both method signatures. This enchantment aligns with the enum refactoring throughout the realm.Also applies to: 30-30
src/Diviner/Windows/LookWindow.cs (3)
78-87: A wise consolidation of console creation magic!The new
CreateLookConsole()method properly centralizes the console creation logic, ensuring consistent sizing and positioning. The cursor positioning at(0, 0)is appropriate for the start of content.
40-74: Magnificent enhancement to tile divination powers!The tile constructor now provides incredibly detailed information about tiles, including material, transparency, walkability, traits, vegetation, and water depth. This wealth of information will greatly aid wizards in understanding their environment, especially with the new terraforming spells.
65-68: Potential null reference in vegetation component access!The vegetation component access could fail if
vegetationis null despite the successfulGetComponentcall. The null-forgiving operator onvegetation!.Plant.Nameis risky.Add proper null verification to your plant-sensing spell:
- if (tile!.GetComponent<PlantComponent>(out var vegetation)) - { - desc.Append("Tile has the following vegetation: ").Append(vegetation!.Plant.Name).AppendLine(); - } + if (tile!.GetComponent<PlantComponent>(out var vegetation) && vegetation?.Plant != null) + { + desc.Append("Tile has the following vegetation: ").Append(vegetation.Plant.Name).AppendLine(); + }Likely an incorrect or invalid review comment.
src/MagusEngine/Core/Entities/Target.cs (6)
1-3: By my staff and scroll! These using statements appear well-ordered.The addition of
System,System.Collections.Generic, andSystem.Linqusing directives aligns with the code's usage patterns and supports the modernization efforts in this mystical targeting system.
157-157: The null-forgiving operators here seem justified, master wizard.Given the context where
_selectedSpelland_casterare checked for range validity just above, these null-forgiving operators appear safe. The logic flow ensures these fields are non-null when this line executes.
166-166: Excellent nullability enhancement! The Item may indeed be null.Changing the return type from
(bool, Item)to(bool, Item?)properly reflects that the item might be null when the operation fails. This improves the API's expressiveness.
206-206: The state transition to IdleMode intrigues me greatly!This change from
LookModetoIdleModesuggests a refinement in the targeting state machine. This aligns with the terrain-altering spell features that may require different idle behaviors.#!/bin/bash # Description: Verify the TargetState enum and understand the IdleMode addition # Expected: Find TargetState enum definition and usage patterns # Search for TargetState enum definition ast-grep --pattern 'enum TargetState { $$$ }' # Search for IdleMode usage across the codebase rg -A 2 -B 2 "IdleMode"
215-215: ```shell
#!/bin/bashVerify if nullable reference types are enabled in MagusEngine project
grep -R "" -n src/MagusEngine/MagusEngine.csproj
--- `81-81`: Ah, let us peer into the arcane scrolls and reveal the true order of the MapLayer realms and the nature of the MaskAllBelow incantation: ```shell #!/bin/bash # Retrieve MapLayer enum definition and its values rg -n 'enum MapLayer' -A 50 -B 0 --color=never # Retrieve the MaskAllBelow implementation to see its behavior rg -n 'MaskAllBelow' -A 5 -B 2 --color=neversrc/MagusEngine/Core/MapStuff/MagiMap.cs (7)
1-4: The using statements have been reorganized with mystical precision!These import adjustments support the codebase modernization and new features. The addition of
MagusEngine.Services.Factorysuggests integration with the new terrain-altering spell system.Also applies to: 9-9, 13-13
102-102: TheuseCachedGridViews: trueparameter brings performance enchantments!This constructor parameter addition suggests optimization for grid view caching, which would benefit the new terrain-altering spells that likely perform frequent tile queries.
212-213: Behold! The collection expressions of modern C# magic!These updates to use collection expressions (
[.. collection]) instead of.ToArray()represent excellent modernization. The syntax is more concise and potentially more performant.Also applies to: 216-217, 218-218
381-381: The EntityRender initialization embraces the new collection syntax beautifully!The modernized collection initialization (
[]) is cleaner and more idiomatic than the previous syntax.
442-442: The random number generator simplification is most elegant!Simplifying from the previous verbose random number generator usage to
GlobalRandom.DefaultRNGimproves code readability while maintaining functionality.
514-514: The collection expressions bring order to this mystical realm!These modernized collection initializations using
[]and[.. collection]syntax represent excellent upgrades to more idiomatic C# code. The spread operator usage in[.. tiles]is particularly elegant.Also applies to: 533-533, 578-578, 676-676, 823-823
32-32: Beware! This inheritance change from GoRogue's Map to a local Map class is significant sorcery.This fundamental change in base class inheritance could have wide-reaching implications. Ensure the local
Mapclass provides all necessary functionality that was previously inherited fromGoRogue.GameFramework.Map.#!/bin/bash # Description: Find the local Map class definition and compare with GoRogue.GameFramework.Map # Expected: Locate the local Map class and understand its relationship to GoRogue's Map # Search for local Map class definition ast-grep --pattern 'class Map $$$' # Search for references to GoRogue.GameFramework.Map to understand the migration rg -A 3 -B 3 "GoRogue\.GameFramework\.Map"
|
|
||
| public int SolidManaCost { get; set; } | ||
| public EffectType EffectType { get; set; } | ||
| public SpellEffectType EffectType { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The permanent enchantment embraces the renamed mystical essence!
Excellent transmutation, dear wizard! The EffectType property now properly channels the renamed SpellEffectType enum, aligning with the grand restructuring of our magical taxonomy.
However, I perceive that this mystical property remains unbound in the constructor's ritual! Unlike its brethren in other effect classes, this EffectType is not given form during the object's creation, which may lead to undefined magical behavior.
Consider initializing this property in the constructor or providing a default value:
[JsonConstructor]
public PermEffect(Actor caster, ISpellEffect enchantment, int nodeCost, string enchantName,
string enchantDesc)
{
Caster = caster;
Enchantment = enchantment;
SolidManaCost = nodeCost;
EnchantName = enchantName;
EnchantDesc = enchantDesc;
+ EffectType = enchantment?.EffectType switch
+ {
+ string effectTypeStr when Enum.TryParse<SpellEffectType>(effectTypeStr, out var result) => result,
+ _ => SpellEffectType.NONE // or appropriate default
+ };
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/MagusEngine/Core/Magic/Effects/PermEffect.cs at line 15, the EffectType
property is declared but not initialized in the constructor, which can cause it
to have an undefined value. To fix this, modify the constructor of the
PermEffect class to assign a default or specific SpellEffectType value to the
EffectType property during object creation, ensuring it is properly initialized
like in other effect classes.
| public MEssionEffect() | ||
| { | ||
| EffectType = EffectType.MEMISSION; | ||
| EffectType = Arquimedes.Enumerators.SpellEffectType.MEMISSION.ToString(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
A properly cast incantation, though it could be more elegant!
The fully qualified namespace usage is wise, and the .ToString() transformation aligns with the new string-based spell effect system. However, this mystical working could be more efficient.
Consider using a constant string instead of repeated .ToString() calls across effect classes:
-EffectType = Arquimedes.Enumerators.SpellEffectType.MEMISSION.ToString();
+EffectType = nameof(SpellEffectType.MEMISSION);Or create a static class with pre-computed string constants to avoid runtime string generation.
📝 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.
| EffectType = Arquimedes.Enumerators.SpellEffectType.MEMISSION.ToString(); | |
| EffectType = nameof(SpellEffectType.MEMISSION); |
🤖 Prompt for AI Agents
In src/MagusEngine/Core/Magic/Effects/MEssionEffect.cs at line 12, the
assignment uses a fully qualified enum with a .ToString() call which generates
the string at runtime. To improve efficiency, replace this with a constant
string representing the effect type or create a static class holding
pre-computed string constants for all spell effect types, then reference the
constant here instead of calling .ToString() repeatedly.
| public LightEffect() | ||
| { | ||
| EffectType = EffectType.LIGHT; | ||
| EffectType = Arquimedes.Enumerators.SpellEffectType.LIGHT.ToString(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Your illumination spell shines true, but with unnecessary complexity!
While your fully qualified incantation Arquimedes.Enumerators.SpellEffectType.LIGHT.ToString() works perfectly, I noticed other spell effects use the simpler SpellEffectType.LIGHT.ToString() pattern. For consistency across our magical grimoire, consider harmonizing the approach!
- EffectType = Arquimedes.Enumerators.SpellEffectType.LIGHT.ToString();
+ EffectType = SpellEffectType.LIGHT.ToString();🤖 Prompt for AI Agents
In src/MagusEngine/Core/Magic/Effects/LightEffect.cs at line 14, replace the
fully qualified enum reference
Arquimedes.Enumerators.SpellEffectType.LIGHT.ToString() with the simpler
SpellEffectType.LIGHT.ToString() to maintain consistency with other spell
effects. Ensure the appropriate using directive for the Arquimedes.Enumerators
namespace is present at the top of the file to support this simplification.
|
|
||
| public ColoredGlyph Appearence { get; } = null!; | ||
| public ColoredGlyph? LastSeenAppereance { get; } | ||
| public ColoredGlyph? LastSeenAppereance { get; private set; } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
A wise encapsulation enchantment has been cast upon the appearance mystique!
The change from public to private setter provides proper protection for the LastSeenAppereance property, preventing unauthorized modifications from external realms.
Whispers from the ancient texts: The spelling "Appereance" lacks an 'a' - it should be "Appearance". Though this likely echoes throughout the magical codebase, future scribes might wish to correct this orthographic anomaly.
🤖 Prompt for AI Agents
In src/MagusEngine/Core/MapStuff/Tile.cs at line 18, the property
LastSeenAppereance has a spelling mistake in its name; it should be
LastSeenAppearance. Rename the property to correct the spelling, ensuring all
references in the codebase are updated accordingly to maintain consistency and
prevent errors.
| // const int defaultSearchRange = 25; | ||
| throw new NotImplementedException("Lazy"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
The FindTypeOfFood method remains unimplemented - a task for future wizards!
The method throws NotImplementedException("Lazy") with commented-out code. While not directly related to the terrain spells, consider whether this functionality might be needed for the complete spell system.
Would you like me to help implement this method or create an issue to track this missing functionality?
Also applies to: 809-810
🤖 Prompt for AI Agents
In src/MagusEngine/Core/MapStuff/MagiMap.cs around lines 795-796 and 809-810,
the FindTypeOfFood method is currently unimplemented and throws
NotImplementedException with a placeholder comment. Decide if this method is
necessary for the spell system; if so, implement the method logic based on the
intended functionality or create a tracking issue to address this missing
feature later. Remove the NotImplementedException and replace it with the proper
implementation or a clear TODO comment referencing the issue.
| { | ||
| Tile? door = GetTileAt<DoorComponent>(room.DoorsPoint[t]); | ||
| room.Doors.Add(door); | ||
| room.Doors.Add(door!); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Another null-forgiving operator - the door shall not be null!
In the context of UpdateRooms(), this assumes that GetTileAt<DoorComponent> always returns a valid door tile. Consider whether this assumption holds true in all map generation scenarios.
🤖 Prompt for AI Agents
In src/MagusEngine/Core/MapStuff/MagiMap.cs at line 176, the code uses a
null-forgiving operator on 'door' when adding it to room.Doors, assuming 'door'
is never null. Review the logic in UpdateRooms() to verify if
GetTileAt<DoorComponent> can indeed return null in any scenario. If it can, add
a null check before adding to room.Doors to avoid potential runtime exceptions;
otherwise, document why null is impossible here and consider removing the
null-forgiving operator for clarity.
| Point[] posRoom = r.RoomPoints; | ||
|
|
||
| for (int x = 0; x < r.Template.Obj.Rows.Length; x++) | ||
| for (int x = 0; x < r!.Template!.Obj!.Rows.Length; x++) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
More null-forgiving operators - the pattern continues throughout this enchanted map!
These operators follow the same pattern seen throughout the file. While they suppress warnings, ensure that the underlying assumptions about non-null values remain valid as the codebase evolves, especially with the new terrain-altering spells that may modify map structure.
Also applies to: 592-592, 619-619, 623-623, 646-646
🤖 Prompt for AI Agents
In src/MagusEngine/Core/MapStuff/MagiMap.cs at lines 586, 592, 619, 623, and
646, the code uses multiple null-forgiving operators (!) to suppress null
warnings on objects like r, Template, and Obj. Review these usages to ensure the
objects are indeed non-null at runtime. Replace the null-forgiving operators
with proper null checks or conditional logic to safely handle potential null
values, preventing runtime exceptions as the map structure changes.
| public void ForceFovCalculation() | ||
| { | ||
| Actor actor = (Actor)ControlledEntitiy; | ||
| Actor actor = (Actor)ControlledEntitiy!; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The forced FOV calculation assumes the ControlledEntity is an Actor!
This null-forgiving operator with cast assumes ControlledEntity is always non-null and always an Actor. Consider adding a type check to prevent runtime exceptions.
-Actor actor = (Actor)ControlledEntitiy!;
+if (ControlledEntitiy is not Actor actor)
+ return;📝 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.
| Actor actor = (Actor)ControlledEntitiy!; | |
| if (ControlledEntitiy is not Actor actor) | |
| return; |
🤖 Prompt for AI Agents
In src/MagusEngine/Core/MapStuff/MagiMap.cs at line 431, the code forcibly casts
ControlledEntity to Actor using the null-forgiving operator, assuming it is
always non-null and of type Actor. To fix this, add a type check using the 'is'
or 'as' operator to verify ControlledEntity is not null and is an Actor before
casting. Handle the case where it is not an Actor or is null to avoid runtime
exceptions.
| return (T)filter.Item; | ||
| } | ||
| return default; | ||
| return default!; |
There was a problem hiding this comment.
This default return with null-forgiving operator seems perilous!
Returning default! for a generic type T where T : IGameObject could return null when the caller expects a valid object. This could lead to null reference exceptions downstream.
-return default!;
+throw new InvalidOperationException($"Entity with ID {id} not found in the map.");Consider throwing an exception instead of returning a potentially null value, as this better communicates the failure condition.
📝 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.
| return default!; | |
| throw new InvalidOperationException($"Entity with ID {id} not found in the map."); |
🤖 Prompt for AI Agents
In src/MagusEngine/Core/MapStuff/MagiMap.cs at line 372, replace the return of
default! with throwing an exception to clearly indicate failure instead of
returning a potentially null generic IGameObject. This prevents null reference
exceptions downstream by explicitly signaling that a valid object could not be
returned.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
2 participants
Summary by CodeRabbit