This code in preferences.cpp:

    // FIXME: Merge these settings with the other ordinary settings
    // in a cPreferences instance! The following lines set global
    // variables, which from the outside is totally unexpected.
    // This must be done in main.cpp instead, where the preferences
    // are loaded!
    pVideo->m_geometry_quality = loader.Get_Video_Geometry_Detail();
    pVideo->m_texture_quality  = loader.Get_Video_Texture_Detail();
    pAudio->m_music_volume     = loader.Get_Audio_Music_Volume();
    pAudio->m_sound_volume     = loader.Get_Audio_Sound_Volume();

sets global variables where it is totally unexpected from the outside. The 4 relevant settings must be included into cPreferences and the corresponding globals must be set in the calling context in main.cpp on game startup.

At some parts SMC classes have constructors that do not actually build a complete object. Instead, they rely on the calling code to actually complete the object. Take for example the constructor of cOverworld:

cOverworld :: cOverworld( void )
{
    m_sprite_manager = new cWorld_Sprite_Manager( this );
    m_animation_manager = new cAnimation_Manager();
    m_description = new cOverworld_description();
    m_layer = new cLayer( this );

    // ...

    m_next_level = 0;

    m_player_start_waypoint = 0;
    m_player_moving_state = STA_STAY;
}

This code is incomplete. Why? It constructs an instance of cOverworld_description and does not initialize it. Why does this matter? Because cOverworld_description’s constructors does something entirely undocumented:

cOverworld_description :: cOverworld_description( void )
{
    m_path = "world_1";
    m_name = _("Unnamed");
    m_visible = 1;
    m_user = 0;

    m_comment = _("Empty");
}

It defaults to loading the world world_1! The consequence of this is that if you create an instance of cOverworld, you are forced to post-initialize it by manually changing the chosen overworld afterwards, but before you call any method on your new object. Check out what cOverworld_Manager::Load_Dir() does:

                // ...
                overworld = new cOverworld();

                // set directory path
                overworld->m_description->m_path = fs::path(current_dir);
                // default name is the path
                overworld->m_description->m_name = path_to_utf8(current_dir.filename());
                // set user
                overworld->m_description->m_user = user_dir;

                objects.push_back( overworld );

                overworld->Load();
            }

Do you see how this first constructs the new cOverworld instance, then post-initializes its m_description member, and then calls the Load() method on the object? This is very bad code quality as it violates one of the main principles of object-oriented programming, the principle of secrecy. As the caller, I do not need to bother on how to create an object, I do not need to cater for an objects internals. I call it’s class’ constructor (potentially with arguments), and have a ready-to-use object.

This code needs to be wiped out entirely and replaced with proper constructors that take arguments to build the instances the way they need to be built.

Valete.

People are constantly having problems with their CEGUI not having the NullRenderer and using a proper XML library directly rather than CEGUI’s (not really good) wrapper is better anyway. Use libxml++ instead.

The following things need to be done in order to remove CEGUI’s XML handling:

• Convert the level loader to libxml++✓
• Convert the world loader to libxml++ ✓
• Convert the savegame loader to libxml++ ✓
• Convert the user preferences loader to libxml++ ✓
• Convert the editor config loader to libxml++ ✓
• Convert the campaign loader to libxml++ ✓
• Convert the level writer to libxml++ ✓
• Convert the world writer to libxml++ ✓
• Convert the savegame writer to libxml++ ✓
• Convert the user preferences writer to libxml++ ✓
• Convert the campaign writer to libxml++ ✓

Since a long time, there is a blue Flyon in SMC that is available for levels, but nobody knows about it, since it is not displayed in the "enemy" menu of the editor. I also observed the same behaviour for the newly-added mecha flyon. Not sure what causes this, though.

The cSavegame and cSave classes are effectively doing two parts of the same job. They should be merged into one single class cSave or so.

See http://secretmaryo.org/phpBB3/viewtopic.php?f=22&t=7821 , patch by deved2:

*** savegame.cpp    2012-03-24 06:31:02.000000000 -0400
--- savegame_new.cpp    2012-04-25 07:25:39.000000000 -0400
*************** void cSave :: Init( void )
*** 177,182 ****
--- 177,186 ----
    m_player_type = 0;
    m_player_state = 0;
    m_itembox_item = 0;
+   if( m_maryo_type == MARYO_GHOST )
+   {
+       m_player_previous_state = 0;
+   }

    // level
    m_level_time = 0;
*************** bool cSavegame :: Save_Game( unsigned in
*** 493,498 ****
--- 497,506 ----
    savegame->m_lives = pLevel_Player->m_lives;
    savegame->m_points = pLevel_Player->m_points;
    savegame->m_player_type = pLevel_Player->m_maryo_type;
+   if( m_maryo_type == MARYO_GHOST )
+   {
+       savegame->m_player_previous_type = pLevel_Player->m_maryo_type_temp_power;
+   }
    savegame->m_player_state = pLevel_Player->m_state;
    savegame->m_itembox_item = pHud_Itembox->m_item_id;

SMC has handled all paths as std::string instances until now. I’ve largely rewritten the resource management system to use boost::filesystem instead (and made pResource_Manager a real resource manager), but enough artifacts remain to have the game crash at start.

Depends on #3.

Currently, each sprite has a member named m_type that denotes what type of sprite it is, e.g. TYPE_FURBALL or TYPE_PARTICLE_EMITTER. In other words, this duplicates the information already present in the C++ type system itself. For example, a check like this:

if (p_sprite->m_type == TYPE_FURBALL)
  // Do things

can be replaced with the following equal code that relies on C++’s type system instead (yes, all furballs have m_type set to TYPE_FURBALL in their constructor):

if (dynamic_cast<cFurball>(p_sprite))
  // Do things

This also has the advantage we can check for intermediate classes like cEnemy instead of having to list all enemy types existing in SMC.

However, I’m not sure where these m_type checks are done in SMC’s code. dynamic_cast is AFAIK a more expensive operation, i.e. it doesn’t perform as good as a simple integer check does (m_type is of type SpriteType, which is an enum, which is an integer). If such checks happen somewhere in SMC’s Update() or Draw() functions, they are called on every (!) frame, which would make a switch to dynamic_cast probably impossible. This needs to be checked; however, if there aren’t such highly frequented checks, we should probably remove SpriteType entirely and completely rely on C++’s type system.

Valete,
Quintus

Currently, the image settings files accept a parameter called type that defines the massivity of the image. This is misnamed and should hint at what it does by being named massivity or so -- I’m not a native english speaker, please make suggestions.

Depends on #42.

Valete,
Quintus

Exactly that. Creating a particle emitter from mruby iss possible, but appeareantly has no effects.

The SMC code contains much code like this (taken from savegame.cpp):

Add_Property(p_node, "level_name", p_level->m_name)

The p_level->m_name thing should go; direct external access to member variables is C-ish struct style, but not proper OOP. There should be getter methods defined instead, so that the code reads:

Add_Property(p_node, "level_name", p_level->Get_Name())

A standard scripting library with some useful tools should be added to SMC, written in pure Ruby. See this forum post for ideas.

When I get time and we've dealt with our other problems, I would like to write a small C++ library similar to PhysicsFS [1] that would provide a simple virtual file system. The advantage of this would be the ability to prevent file name collision. For instance, Package A might add an image called cherry.png, and Package B might provide the same image. The virtual file system only mounts relevant data to a given level, so a level in Package A would just mount Package A and the core file set.

Additionally, if we ever add new images to the core file set, we don't have to worry about the packages overwriting them, since they just override them in the virtual file system. If the maintainer of one of the packages wants access to that, than they can voluntarily, and without much trouble, edit their own file name so they can see the file they override. If they have no want to access that overridden image, they have no need to update the package.

This would function just well with resource packs. I like the idea that people can create resource packages that level editors can add without the mainline having to add them manually. With this mechanism, resource pack collisions wouldn't be nearly as much of an issue, and different level packs could use different versions of the same resource pack without any collision problems.

One of the features I want to add to the C++ library is the ability to support transparent reads from archives (the same as C++), so that we can change the package format we designed with SMC-GET to simple ZIP files (required instead of tarballs for better random access) or 7Z files that people can easily create and just drop into SMC without having to extract anything. It would also help save disk space by compressing the level and script files.

The reason I would like to write a custom C++ library instead of using PhysicsFS is for ease of integration into SMC. I'd write it as an external system that we could include similarly to mruby so it wouldn't much count as an external library. Plus, I've been interested in writing my own virtual file system library for awhile. ;)

[1] http://icculus.org/physfs/

The collision handlers (Handle_Collision_Player(), Handle_Collision_Enemy(), etc.) are currently public methods. They are not intended to be called from the outside of a sprite, but they do have an inheritance chain (they’re virtual), so they should be protected.

Valete,
Quintus

I just noticed that each and every single enemy contains this code (this one taken from the furball):

void cFurball :: Update( void )
{
  // ...
  if( !m_valid_update || !Is_In_Range() )
  {
    return;
  }

  // ...
}

This checks whether an update shall be executed on the sprite as per internal conditions of the sprite (m_valid_update) and as per the current camera update range (Is_In_Range()). That code is the very same for nearly all the sprites, and should just be moved way up in the inheritance hierarchy.

Generally, I have the feeling that especially the enemies contain a lot of duplicated code that should just be moved to cEnemy. I have to re-check this, tough.

Valete,
Quintus

Currently, the credits scroll infinitely, again and again (and with graphic errors towards the "special thanks"). The credits should exit to the main game menu when done, there’s no real reason why they should keep repeating over and over.

Vale,
Quintus

Entering a pipe will stop the invincibility but the music will continue to play until it reaches the end of the song (which is far longer than the star lasts).
The pipe won't stop the invincibility if it leads to another area in the same level, but if the pipe goes to another level, the glitch will occur.

There’s a good amount of Lua functionality which hasn’t been brided over to mruby yet.

Introducing Crabmaster's idea and graphics from this thread: http://secretmaryo.org/phpBB3/viewtopic.php?f=10&t=3900
It's a static enemy that spawns smaller non-static enemies when Maryo gets within (a specified) range.
Quite similar to SMW's fireball plant. https://dl.dropboxusercontent.com/u/22806246/smw_enemies_sheet.png

The m_level_ends_if_killed member currently only exists for specific enemies, namely the furball boss and the turtle boss. While for normal gameplay this is fine, for scripting it would be much more interesting to have the level end when any given enemy is killed, e.g. a spika or so. This requires the m_level_ends_if_killed member to be moved directly into the cEnemy class and merging the slightly different code currently in use in the cFurball and cTurtleBoss classes into one common place.

Vale,
Quintus

If there is no config file in ~/.smc/, SMC will save the config right next to the executable instead of in the SMC config directory. Seems like this was intentional, but why?

Krushes should have an option to be set to small. That is, you double-click on the Krush, and next to the direction box there should be an option to make it small.

This has so often been requested, one should really add that to SMC.

Valete,
Quintus

Before today XmlAttributes::fetch() wasn’t templated, but only worked with strings, requiring an absolutely unnecessry conversion of the defaultvalue to a string before it could be converted back. Example:

Old:

// audio/random_sound.cpp
Set_Volume_Reduction_Begin(string_to_float(attributes.fetch("volume_reduction_begin", float_to_string(m_volume_reduction_begin))));

New:

// audio/random_sound.cpp
Set_Volume_Reduction_Begin(attributes.fetch<float>("volume_reduction_begin", m_volume_reduction_begin));

This has been changed for many sprites, but especially the enemies have not been changed yet. You’ll always find these calls in the constructors of cSprite subclasses whith this signature (yes cRandom_Sound is a subclass of cSprite):

// audio/random_sound.h
cRandom_Sound( XmlAttributes &attributes, cSprite_Manager *sprite_manager );

As this is not only nicer to read, but also improves loading performance (the conversion of the defaultvalue is missing), this should be changed soon.

Vale,
Quintus

The SpriteType enum is mainly used for determining the "class" of an object without having to look at it’s C++ class information (mainly for performance reasons I guess). It however appears to be cluttered with the massivity information:

    TYPE_PASSIVE = 44,
    TYPE_FRONT_PASSIVE = 45,
    TYPE_MASSIVE = 46,
    TYPE_HALFMASSIVE = 5,
    TYPE_CLIMBABLE = 47,

which is redundant with the MassiveType enum further below in game_core.hpp:

enum MassiveType
{
    MASS_PASSIVE = 0,
    MASS_MASSIVE = 1,
    MASS_HALFMASSIVE = 2,
    MASS_CLIMBABLE = 3
};

The massivity information should be removed from the SpriteType enum and SMC should completely rely on the MassiveType enum and it’s corresponding m_massive_type member of cSprite.

Valete,
Quintus

It appears that SMC appears to freeze when a user click on something that would bring up a dialog box or window, such as when clicking options or changing the speedfactor, or (edit) exiting the level editor. It however (in my experience) only does not occur when selecting a level.

Edit: Here are two videos demonstrating the issue.

http://www.youtube.com/watch?v=LYso0glghTw
http://www.youtube.com/watch?v=oPPKMV9RVHE

The compiled-in fixed resource path is now history. Instead, the resource manager determines the current executable’s path and retrieves the data directory from there (unless you use the FIXED_DATA_PATH compilation option to force a specific directory). However, there are still far too many references to DATA_PATH (whose definition I’ve already removed) in the sourcecode which must go.

Just like the title says, the music that plays when you complete a level that you selected directly from the main menu, not from an over world, will infinitely repeat. This should return to the main theme music.

SMC currently stores it’s game state as in-memory XML. WTF?

// Game Action data
extern CEGUI::XMLAttributes Game_Action_Data_Start;
extern CEGUI::XMLAttributes Game_Action_Data_Middle;
extern CEGUI::XMLAttributes Game_Action_Data_End;

This is never written out to disk, hence there is absolutely no need to use XML for this. These variables should all be replaced with something based on std::map. Maybe extract the type-conversion code from SMC’s own XmlAttributes and work with subclasses? Or just rename it to something more generic?

There’s some initial code on Cape Maryo, it should probably be extended. See also this forum thread.

Currently, there’s a huge function Create_Level_Object_From_XML() in level.cpp that contains a giant if clause checking for every possible object in the XML level file. As it appears, there’s also an unknown amount of decentralised virtual Load_From_XML() methods that are never used. These need to be synchronised with the centralised loading code, which should then be kicked out in favour of the decentralised system which allows for a much more easy addition/removal of new/old elements.

This should probably solved together with #9.

Warnings should go to standard error, not to standard output. SMC often uses printf(), which a) prints to standard output and b) is legacy C code if you don’t need to formatting functionality (in which case fprintf() should be used with the standard error stream here).

Shells that are just lying around, without army inside. Maryo can pick them up, let them roll, stop them, but there’s no enemy in them. Just an empty shell.

Currently, SMC has no concept of exception handling. It prints wealths of warnings, whose conditions may even crash the game later on and which make debugging unneccessarily hard because you have to manually determine where the warning came from rather than just having GDB figure out where an exception popped up from. A proper exception system should use proper exception classes which useful error messages and a good exception hierarchy.

I’ve heard that on Windows, you don’t have a package manager. Really poor guys. So we need to provide them with an installer; CMake has native support for building these, called CPack.

Valete,
Quintus

Currently, SMC only officially supports CEGUI 0.7.x. It’s time to move on!

• Depends on #9

Steps to reproduce:
Load a level that has a script + timer. Press Esc, and try to start another level (can even be the same one with the script). If it does not crash here, just Esc and pick one again. It should happen this time.
The error it spawns is:

terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::lock_error> >'
  what():  boost: mutex lock failed in pthread_mutex_lock: Invalid argument
Aborted

Seems like someone forgot to stop timers when exiting a level...

We need a file selector that's much easier for people who are not quite familiar with computer mechanics, as opposed to the filename/here "selector".

There are already armadillo shells that can be carried, so this shouldn't be too hard.
Maybe it should have a bool, though, "carry-able".

A (possibilty incomplete) list of what should be implemented on the scripting side:

• Quick’n’easy way to create a static sprite ✓
• mruby classes for all existing enemies ✓
• mruby classes for all existing powerups ✓
• mruby classes for all existing boxes✓
• Helper classes for common static sprites (like door + levelexit)
• Scripting paths for moving/falling platforms ✓
• Scripting timers ✓
• Level save and load events ✓
• The infamous touch event ✓
• Allow scripts to hijack Maryo’s movements
• Allow scripts to apply powerups directly to Maryo ✓
• Allow scripts to display messages ✓

Feel free to add to this issue what you would like to see in scripting.

Valete.

SDL 2 is out. We should support it.

Using true and false instead of the numbers 1 and 0 makes code far more readable.

Currently, powerup.cpp and powerup.h contain multiple classes. This is bad style; there should be a directory powerups/ that contains separate files for cMushroom, cFirePlant, etc.

Steps to reproduce:
Select any world, press Esc, go to "Save", save the game, press Esc, try to enter either the Load menu or Save menu and the game will crash.
This happens because SMC displays the level's current time in the Save/Load menus, but since the overworld does not have time, it can't find a time to display.

Pip, the worm! See this forum thread for ideas.

• It is a two-hit enemy.
• On first hit, it splits into two small pips.
• On second hit, it dies and gives Maryo a large Y acceleration (must be greater than the power jump)
• Can only be defeated by non-small Maryo

mruby is better. Lua must go.

Currently only one path is possible on the overworld. We should have the possibility to unravel paths depending on which level exit the user took in a level.

The path handling in overworlds currently is confusing and hard to predict. Instead of working with lines which may or may not work we should have some definite list which waypoint can unlock which other waypoints. The necessary directions the player must go can most likely be determined by looking at the positions of unlocked waypoints.

Or in short, rewrite the entire path handling code for overworlds.

Currently the cFurball class covers the normal brown, icy blue and giant boss furball. These furballs however do not only have different colors and sizes, they behave differently and should be separate classes that inheritc from a common cFurball class. Code like

if (m_type == TYPE_FURBALL_BOSS) {
  // ...
}

must vanish, this is not OOP.

Vale,
Quintus

In nearly every .cpp source file of SMC there is a line like this:

namespace fs = boost::filesystem;

This is annoying. They should all be removed and replaced with one single line in global_game.h, that is thereby included into all sourcefiles.

Vale,
Quintus