if (!_sprite)
if (!_sprite)
if (!_sprite)
if (!_sprite)
std::vector<Cell*> _neighbors;
The set of neighbors is fixed, right? Like, 8 of them, not more?
The vector will allocate memory separately. Use a fixed-size array instead.
I don’t know what you are using for the matrix. It may, however, be overly complex. If you use a flattened linear vector instead of a 2D array (or vector of vectors) you can move around to neighbors quickly by using strides. So, you might not need to store this member at all, but will build it on the fly with simple arithmetic.
std::vector<Cell*> _neighbors;
The set of neighbors is fixed, right? Like, 8 of them, not more?
The vector will allocate memory separately. Use a fixed-size array instead.
I don’t know what you are using for the matrix. It may, however, be overly complex. If you use a flattened linear vector instead of a 2D array (or vector of vectors) you can move around to neighbors quickly by using strides. So, you might not need to store this member at all, but will build it on the fly with simple arithmetic.
Sounds like quite an endeavor! I miss Sim City 2000. I even bought an Android version for 5ドル and then they took it away!
Good luck on this project.
Writing identifiers with a leading underscore is a bad idea, and is generally dissuaded as a style. Note that it is legal for member names if (and only if) the next character is not a capital letter or another underscore.
You have a default constructor for Cell which is empty! So what about all the raw pointer data members?
NL.9: Use ALL_CAPS for macro names only
You will need to define keyTileMap in one CPP file; you should move the huge initialization stuff to there, too.
In the constructor, use the member init list, not assignments in the body of the function. Some of those can be default initializers on the data members (tileID, elevatedTilePosition).
Don’t define an empty destructor. Let the compiler supply it; it will generate better code that way. If you must, just declare it as =default in the class.
Many of the simple functions should be inline in the class definition. e.g. { return sprite; }.
void Cell::setNeighbors(std::vector<Cell*> neighbors)
{
_neighbors = std::move(neighbors);
}
OK, here is a tricky one! Do not treat this as a "sink" parameter! That is only for when you know a new object needs to be created (e.g. in constructors), not for when an existing object is reused.
You are forcing a full copy of neighbors complete with a block of memory, and deleting the memory block that _neighbors already held. Regular assignment would reuse this existing block of memory, if it fit.
I’ve seen a presentation with benchmarks, but I don’t recall who gave it now.
_sprite = new Sprite(_tileID, _isoCoordinates);
⧺C.149 — no naked new or delete.
You should probably make this a unique_ptr as a drop-in replacement without otherwise changing the architecture.
if (_sprite != nullptr)
Don’t write explicit tests against nullptr. Use the contextual conversion to bool, or in this case, the operator!.
if (!_sprite)
for (int i = 0; i < _neighbors.size(); i++)
Don’t go through the collection by index. Look how many times you use neighbors[i] in the block of code! Use the range-for construct to iterate directly over the vector!
for (auto n : _neighbors) {
⋮
(Note that if you change your vector to hold values rather than pointers, you will make that auto&)
Your ELEVATED constants are bitmapped, right? So you can combine them.
(_elevatedTilePosition & ELEVATED_LEFT) && (_elevatedTilePosition & ELEVATED_RIGHT)
constexpr auto LEFT_and_RIGHT = ELEVATED_LEFT|ELEVATED_RIGHT;
(_elevatedTilePosition & LEFT_and_RIGHT == LEFT_and_RIGHT)
The && is a short-circuit operation, so the code has to jump. Jumps are slow. Testing that both bits are set is the same speed as testing one!