Skip to content

Commit

Permalink
Fix memory leaks and code quality in autotile code (#2591)
Browse files Browse the repository at this point in the history
Fixes various code quality related issues in autotile code. Also fixes all memory leaks found by AddressSanitizer. Game doesn't use heap allocation anymore where it's not neccessary and uses smart pointers where possible/required.
  • Loading branch information
mrkubax10 authored Aug 21, 2023
1 parent 4a5bdc0 commit e105412
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 109 deletions.
67 changes: 21 additions & 46 deletions src/supertux/autotile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <http://www.gnu.org/licenses/>.

#include <bitset>

#include "supertux/autotile.hpp"

//#include "supertux/autotile_parser.hpp"
#include <bitset>

#include "util/log.hpp"

// AutotileMask.

Expand All @@ -34,17 +34,11 @@ AutotileMask::matches(uint8_t mask, bool center) const
return mask == m_mask && center == m_center;
}

uint8_t
AutotileMask::get_mask() const
{
return m_mask;
}

// Autotile.

Autotile::Autotile(uint32_t tile_id, std::vector<std::pair<uint32_t, float>> alt_tiles, std::vector<AutotileMask*> masks, bool solid) :
Autotile::Autotile(uint32_t tile_id, const std::vector<std::pair<uint32_t, float>>& alt_tiles, const std::vector<AutotileMask>& masks, bool solid) :
m_tile_id(tile_id),
m_alt_tiles(std::move(alt_tiles)),
m_alt_tiles(alt_tiles),
m_masks(std::move(masks)),
m_solid(solid)
{
Expand All @@ -55,20 +49,14 @@ Autotile::matches(uint8_t num_mask, bool center) const
{
for (auto& l_mask : m_masks)
{
if (l_mask->matches(num_mask, center))
if (l_mask.matches(num_mask, center))
{
return true;
}
}
return false;
}

uint32_t
Autotile::get_tile_id() const
{
return m_tile_id;
}

uint32_t
Autotile::pick_tile(int x, int y) const
{
Expand Down Expand Up @@ -116,35 +104,28 @@ uint8_t
Autotile::get_first_mask() const
{
if (!m_masks.empty())
return m_masks[0]->get_mask();
return m_masks[0].get_mask();
return 0;
}

std::vector<std::pair<uint32_t, float>>
Autotile::get_all_tile_ids() const
{
return m_alt_tiles;
}

bool
Autotile::is_solid() const
{
return m_solid;
}


// AutotileSet.

std::vector<AutotileSet*>* AutotileSet::m_autotilesets = new std::vector<AutotileSet*>();
std::vector<std::unique_ptr<AutotileSet>> AutotileSet::m_autotilesets;

AutotileSet::AutotileSet(std::vector<Autotile*> tiles, uint32_t default_tile, std::string name, bool corner) :
m_autotiles(std::move(tiles)),
AutotileSet::AutotileSet(const std::vector<Autotile*>& tiles, uint32_t default_tile, const std::string& name, bool corner) :
m_autotiles(tiles),
m_default(default_tile),
m_name(std::move(name)),
m_name(name),
m_corner(corner)
{
}

AutotileSet::~AutotileSet()
{
for (Autotile* autotile : m_autotiles)
delete autotile;
}

/*
AutotileSet*
AutotileSet::get_tileset_from_tile(uint32_t tile_id)
Expand Down Expand Up @@ -210,7 +191,7 @@ AutotileSet::get_autotile(uint32_t tile_id,
if (top_left) num_mask = static_cast<uint8_t>(num_mask + 0x80);
}

for (auto& autotile : m_autotiles)
for (auto* autotile : m_autotiles)
{
if (autotile->matches(num_mask, center))
{
Expand All @@ -221,12 +202,6 @@ AutotileSet::get_autotile(uint32_t tile_id,
return center ? get_default_tile() : 0;
}

uint32_t
AutotileSet::get_default_tile() const
{
return m_default;
}

bool
AutotileSet::is_member(uint32_t tile_id) const
{
Expand Down Expand Up @@ -258,7 +233,7 @@ AutotileSet::is_solid(uint32_t tile_id) const
if (!is_member(tile_id))
return false;

for (auto& tile : m_autotiles)
for (auto* tile : m_autotiles)
{
if (tile->get_tile_id() == tile_id)
{
Expand Down Expand Up @@ -286,7 +261,7 @@ AutotileSet::is_solid(uint32_t tile_id) const
uint8_t
AutotileSet::get_mask_from_tile(uint32_t tile) const
{
for (auto& autotile : m_autotiles)
for (auto* autotile : m_autotiles)
{
if (autotile->is_amongst(tile)) {
return autotile->get_first_mask();
Expand All @@ -307,7 +282,7 @@ AutotileSet::validate() const
uint32_t tile_nonsolid = 0; // Relevant only for non-corner autotiles.
uint32_t tile_with_that_mask = 0; // Used to help users debug.

for (auto& autotile : m_autotiles)
for (auto* autotile : m_autotiles)
{
if (autotile->matches(num_mask, true))
{
Expand Down
40 changes: 13 additions & 27 deletions src/supertux/autotile.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,11 @@
#ifndef HEADER_SUPERTUX_SUPERTUX_AUTOTILE_HPP
#define HEADER_SUPERTUX_SUPERTUX_AUTOTILE_HPP

#include <algorithm>
#include <memory>
#include <stdint.h>
#include <string>
#include <algorithm>

#include "math/rect.hpp"
#include "math/rectf.hpp"
#include "math/size.hpp"
#include "object/path_object.hpp"
#include "object/path_walker.hpp"
#include "squirrel/exposed_object.hpp"
#include "scripting/tilemap.hpp"
#include "supertux/game_object.hpp"
#include "video/color.hpp"
#include "video/flip.hpp"
#include "video/drawing_target.hpp"
#include <vector>

class AutotileMask final
{
Expand All @@ -41,29 +30,25 @@ class AutotileMask final

bool matches(uint8_t mask, bool center) const;

uint8_t get_mask() const;
uint8_t get_mask() const { return m_mask; }

private:
uint8_t m_mask;
bool m_center; // m_center should *always* be the same as the m_solid of the corresponding Autotile

private:
AutotileMask(const AutotileMask&) = delete;
AutotileMask& operator=(const AutotileMask&) = delete;
};

class Autotile final
{
public:
Autotile(uint32_t tile_id,
std::vector<std::pair<uint32_t, float>> alt_tiles,
std::vector<AutotileMask*> masks,
const std::vector<std::pair<uint32_t, float>>& alt_tiles,
const std::vector<AutotileMask>& masks,
bool solid);

bool matches(uint8_t mask, bool center) const;

/** @deprecated Returns the base tile ID. */
uint32_t get_tile_id() const;
uint32_t get_tile_id() const { return m_tile_id; }

/** Picks a tile randomly amongst the possible ones for this autotile. */
uint32_t pick_tile(int x, int y) const;
Expand All @@ -75,15 +60,15 @@ class Autotile final
uint8_t get_first_mask() const;

/** Returns all possible tiles for this autotile */
std::vector<std::pair<uint32_t, float>> get_all_tile_ids() const;
const std::vector<std::pair<uint32_t, float>>& get_all_tile_ids() const { return m_alt_tiles; }

/** Returns true if the "center" bool of masks are true. All masks of given Autotile must have the same value for their "center" property.*/
bool is_solid() const;
bool is_solid() const { return m_solid; }

private:
uint32_t m_tile_id;
std::vector<std::pair<uint32_t, float>> m_alt_tiles;
std::vector<AutotileMask*> m_masks;
std::vector<AutotileMask> m_masks;
bool m_solid;

private:
Expand All @@ -98,7 +83,8 @@ class AutotileSet final
//static AutotileSet* get_tileset_from_tile(uint32_t tile_id);

public:
AutotileSet(std::vector<Autotile*> autotiles, uint32_t default_tile, std::string name, bool corner);
AutotileSet(const std::vector<Autotile*>& autotiles, uint32_t default_tile, const std::string& name, bool corner);
~AutotileSet();

/** Returns the ID of the tile to use, based on the surrounding tiles.
* If the autotileset is corner-based, the top, left, right, bottom and
Expand All @@ -112,7 +98,7 @@ class AutotileSet final
) const;

/** Returns the id of the first block in the autotileset. Used for erronous configs. */
uint32_t get_default_tile() const;
uint32_t get_default_tile() const { return m_default; }

/** true if the given tile is present in the autotileset */
bool is_member(uint32_t tile_id) const;
Expand All @@ -133,7 +119,7 @@ class AutotileSet final
void validate() const;

public:
static std::vector<AutotileSet*>* m_autotilesets;
static std::vector<std::unique_ptr<AutotileSet>> m_autotilesets;

private:
std::vector<Autotile*> m_autotiles;
Expand Down
30 changes: 14 additions & 16 deletions src/supertux/autotile_parser.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// SuperTux
// Copyright (C) 2008-2020 A. Semphris <[email protected]>,
// Matthias Braun <[email protected]>,
// Ingo Ruhnke <[email protected]>
// Copyright (C) 2020 A. Semphris <[email protected]>
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -29,7 +27,7 @@
#include "util/reader_mapping.hpp"
#include "util/file_system.hpp"

AutotileParser::AutotileParser(std::vector<AutotileSet*>* autotilesets, const std::string& filename) :
AutotileParser::AutotileParser(std::vector<std::unique_ptr<AutotileSet>>& autotilesets, const std::string& filename) :
m_autotilesets(autotilesets),
m_filename(filename),
m_tiles_path()
Expand Down Expand Up @@ -71,7 +69,7 @@ AutotileParser::parse()
void
AutotileParser::parse_autotileset(const ReaderMapping& reader, bool corner)
{
std::vector<Autotile*>* autotiles = new std::vector<Autotile*>();
std::vector<Autotile*> autotiles;

std::string name = "[unnamed]";
if (!reader.get("name", name))
Expand All @@ -91,28 +89,28 @@ AutotileParser::parse_autotileset(const ReaderMapping& reader, bool corner)
if (iter.get_key() == "autotile")
{
ReaderMapping tile_mapping = iter.as_mapping();
autotiles->push_back(parse_autotile(tile_mapping, corner));
autotiles.push_back(parse_autotile(tile_mapping, corner));
}
else if (iter.get_key() != "name" && iter.get_key() != "default")
{
log_warning << "Unknown symbol '" << iter.get_key() << "' in autotile config file" << std::endl;
}
}

AutotileSet* autotileset = new AutotileSet(*autotiles, default_id, name, corner);
std::unique_ptr<AutotileSet> autotileset = std::make_unique<AutotileSet>(autotiles, default_id, name, corner);

if (g_config->developer_mode)
{
autotileset->validate();
}

m_autotilesets->push_back(autotileset);
m_autotilesets.push_back(std::move(autotileset));
}

Autotile*
AutotileParser::parse_autotile(const ReaderMapping& reader, bool corner)
{
std::vector<AutotileMask*> autotile_masks;
std::vector<AutotileMask> autotile_masks;
std::vector<std::pair<uint32_t, float>> alt_ids;

uint32_t tile_id;
Expand Down Expand Up @@ -143,11 +141,11 @@ AutotileParser::parse_autotile(const ReaderMapping& reader, bool corner)

if (corner)
{
parse_mask_corner(mask, &autotile_masks);
parse_mask_corner(mask, autotile_masks);
}
else
{
parse_mask(mask, &autotile_masks, solid);
parse_mask(mask, autotile_masks, solid);
}
}
else if (iter.get_key() == "alt-id")
Expand Down Expand Up @@ -177,11 +175,11 @@ AutotileParser::parse_autotile(const ReaderMapping& reader, bool corner)
}
}

return new Autotile(tile_id, alt_ids, autotile_masks, !!solid);
return new Autotile(tile_id, alt_ids, autotile_masks, solid);
}

void
AutotileParser::parse_mask(std::string mask, std::vector<AutotileMask*>* autotile_masks, bool solid)
AutotileParser::parse_mask(std::string mask, std::vector<AutotileMask>& autotile_masks, bool solid)
{
if (mask.size() != 8)
{
Expand Down Expand Up @@ -224,12 +222,12 @@ AutotileParser::parse_mask(std::string mask, std::vector<AutotileMask*>* autotil

for (uint8_t val : masks)
{
autotile_masks->push_back(new AutotileMask(val, solid));
autotile_masks.push_back(AutotileMask(val, solid));
}
}

void
AutotileParser::parse_mask_corner(std::string mask, std::vector<AutotileMask*>* autotile_masks)
AutotileParser::parse_mask_corner(std::string mask, std::vector<AutotileMask>& autotile_masks)
{
if (mask.size() != 4)
{
Expand Down Expand Up @@ -272,7 +270,7 @@ AutotileParser::parse_mask_corner(std::string mask, std::vector<AutotileMask*>*

for (uint8_t val : masks)
{
autotile_masks->push_back(new AutotileMask(val, true));
autotile_masks.push_back(AutotileMask(val, true));
}
}

Expand Down
Loading

0 comments on commit e105412

Please sign in to comment.