Document villa furniture#1076
Conversation
Kuruyia
left a comment
Kuruyia
left a comment
There was a problem hiding this comment.
Only minor stuff here, this looks very clean!
There was a problem hiding this comment.
suggestion: I think this can be merged into villa_furniture.h.
| typedef struct VillaPersistedFeature { | ||
| int persistedFeature; | ||
| } VillaPersistedFeature; |
There was a problem hiding this comment.
suggestion: Rename to VillaPersistedData
I initially wanted to have the PersistedMapFeatures_GetBuffer-related structs all end in PersistedData, but I know some of them already fell through the cracks.
| typedef struct VillaPersistedFeature { | |
| int persistedFeature; | |
| } VillaPersistedFeature; | |
| typedef struct VillaPersistedData { | |
| int persistedFeature; | |
| } VillaPersistedData; |
There was a problem hiding this comment.
Updated for consistency, but I disagree with this naming convention on grounds that ending any struct name with "-Data" is inherently redundant, outside of very specific circumstances that I don't think applies here.
There was a problem hiding this comment.
The rationale behind suffixing this family of structs with Data is that their content are directly written to the save data. So this is a pretty generic system.
I'm not really attached to that naming convention tho'. We can discuss about that for another PR. As long as the naming stays consistent :)
| #define MAP_OBJECT_COORD_TO_FX32(coord) ((coord << 4) * FX32_ONE) + (MAP_OBJECT_TILE_SIZE >> 1) | ||
| #define ALSO_MAP_OBJECT_COORD_TO_FX32(coord) ((coord << 4) * FX32_ONE) |
There was a problem hiding this comment.
question: Shouldn't MAP_OBJECT_COORD_TO_FX32 be MAP_OBJECT_COORD_TO_FX32_CENTERED?
| #define MAP_OBJECT_COORD_TO_FX32(coord) ((coord << 4) * FX32_ONE) + (MAP_OBJECT_TILE_SIZE >> 1) | |
| #define ALSO_MAP_OBJECT_COORD_TO_FX32(coord) ((coord << 4) * FX32_ONE) | |
| #define MAP_OBJECT_COORD_TO_FX32(coord) ((coord << 4) * FX32_ONE) | |
| #define MAP_OBJECT_COORD_TO_FX32_CENTERED(coord) ((coord << 4) * FX32_ONE) + (MAP_OBJECT_TILE_SIZE >> 1) |
There was a problem hiding this comment.
Additionally, we can improve those macros a bit:
#define MAP_OBJECT_COORD_TO_FX32(coord) (coord * MAP_OBJECT_TILE_SIZE)
#define MAP_OBJECT_COORD_TO_FX32_CENTERED(coord) (MAP_OBJECT_COORD_TO_FX32(coord) + MAP_OBJECT_TILE_SIZE / 2)| #define POKEPLATINUM_STRUCT_VILLA_PERSISTED_FEATURE_H | ||
|
|
||
| typedef struct VillaPersistedFeature { | ||
| int persistedFeature; |
There was a problem hiding this comment.
question: Is this actually referenced anywhere? If not, I think you can rename it to dummy.
| int persistedFeature; | |
| int dummy; |
| return FALSE; | ||
| } | ||
|
|
||
| static const VillaFurnitureCollision sVillaFurnitureCollision[FURNITURE_SLOTS] = { |
There was a problem hiding this comment.
suggestion: Seeing that it also holds the position of the furniture, the name can be simplified.
| static const VillaFurnitureCollision sVillaFurnitureCollision[FURNITURE_SLOTS] = { | |
| static const VillaFurniture sVillaFurnitures[FURNITURE_SLOTS] = { |
There was a problem hiding this comment.
VillaFurniture is already defined as the name of the enum, the compiler doesn't like it when that symbol gets redefined as a struct here.
There was a problem hiding this comment.
Would it make sense to rename this enum to VillaFurnitureType?
There was a problem hiding this comment.
This whole thing comes together pretty nicely when the enum is VillaFurnitureID.
CharlesFolz4
force-pushed
the
VillaFurniture
branch
2 times, most recently
from
823b48a to
98cb9ce
Compare
|
|
||
| typedef struct VillaDynamicMapData { | ||
| FieldSystem *fieldSystem; | ||
| VillaPersistedData *persistedFeature; |
There was a problem hiding this comment.
polish: Consistency with the rename that was done.
| VillaPersistedData *persistedFeature; | |
| VillaPersistedData *persistedData; |
| void Villa_DynamicMapFeaturesInit(FieldSystem *fieldSystem) | ||
| { | ||
| PersistedMapFeatures *persistedFeatures = MiscSaveBlock_GetPersistedMapFeatures(FieldSystem_GetSaveData(fieldSystem)); | ||
| VillaPersistedData *villaPersistedFeatures = PersistedMapFeatures_GetBuffer(persistedFeatures, DYNAMIC_MAP_FEATURES_VILLA); |
There was a problem hiding this comment.
polish: Consistency with the rename that was done.
| VillaPersistedData *villaPersistedFeatures = PersistedMapFeatures_GetBuffer(persistedFeatures, DYNAMIC_MAP_FEATURES_VILLA); | |
| VillaPersistedData *persistedData = PersistedMapFeatures_GetBuffer(persistedFeatures, DYNAMIC_MAP_FEATURES_VILLA); |
CharlesFolz4
force-pushed
the
VillaFurniture
branch
2 times, most recently
from
4c5e81e to
0610030
Compare
| } FurnitureCollisionBoundingBox; | ||
|
|
||
| typedef struct VillaFurniture { | ||
| u16 furnitureID; |
There was a problem hiding this comment.
nitpick: It can be even nicer :)
| u16 furnitureID; | |
| u16 id; |
There was a problem hiding this comment.
Given that the index of sVillaFurnitures doesn't always match up with the furnitureIDs, I think it's better to keep this field explicit to avoid any potential confusion.
There was a problem hiding this comment.
I think the initial Type suggestion would be more appropriate here then. Also because I see that in the mentioned array there are multiple entries with the same furnitureID.
There was a problem hiding this comment.
You're right. Updated.
CharlesFolz4
force-pushed
the
VillaFurniture
branch
from
0610030 to
1ba6696
Compare
3 participants
Document villa furniture