It's time, in my humble opinion, for the first ever crystals feature freeze!

In a bid to ensure all code contributed to crystals so far is of optimal quality (both written and compiled), normal progress on crystals is going to halt in order to allow for the work currently in staging to be properly reviewed and prepared for master.

The code review will take four forms:

• Intensive valgrind testing, to ensure that there are no noticeable memory leaks or errors.
• Refactoring of code to increase consistency/coherency and decrease coupling.
• Consistency-checking of code style to ensure everything follows crystals's standards (some style guides may need writing up).
• Improvement to documentation so that new coders can more easily jump in.

No more feature pulls will be allowed into staging between now and the closure of this issue. The final push to master in this space will likely be tagged as version 0.01.

Hopefully, there will be a more detailed action plan going up soon.

Thanks to everyone who has worked on crystals so far! Even though crystals isn't capable of much yet, it's a start and I think the amount of work that has gone into it despite university and other more important issues is something to be proud of.

~ Matt

PS: Java map editor will be going up shortly.

As per something barrucadu mentioned a few days ago, I'm putting this to a sort-of vote/comment/etc.

Proposal

The proposal is to stop targeting a pedantic flavour of C89, and instead target the GNU dialect of C, and also assume the presence of glibc.

Specifically, the change proposed is to:

• Change CFLAGS in Makefile from
  CFLAGS := -ansi -pedantic -O2 -g3 -ggdb -DDEFMODPATH="\"$(MODPATH)\"" $(WARN)
  to
  CFLAGS := -O2 -g3 -ggdb -DDEFMODPATH="\"$(MODPATH)\"" $(WARN)

Precedents

• Inclusion of glib2 - the project's moving to a much more "don't reinvent the wheel" style of development, with the idea of using as much time-saving functionality as possible. (Better have a finished product that works on our systems as soon as possible than never finish one because of anal adherence to portability on every system imaginable!)

Pros

• Would allow GNU (including some C99) extensions to the C language. Examples:
  • Variable-length arrays
  • Variadic macros (probably useful for debug prints)
  • Attributes for functions etc.
• Would allow GNU-specific functions in glibc - the main one I'm looking forward to would be asprintf.

Cons

• Would likely break Visual C++ (not that that's the end of the world as we know it, we can just use MinGW etc.);
• Might break portability a bit, but most targets have a GNU compiler so...

Personal stance

I'm happy either way. I know that other developers are probably strongly in favour of this, though. =D

The tests in src/tests/ are unusable, because of the unified error message. There will be even more functions, types and whatever which makes it impossible to use those tests unless we compile everything into them and setup up a complete environment (g_config, etc).

The only option I can think of is to remove them. They were usefull for testing the specific parts of our game before we created dependencies on other files. But now they're broken and useless (and I can't fix them).

Here are a few things I thought there should be a bit of discussion on:

Idioms

Error-aware function calls

The current idiom/style is:

    if (do_something () == FAILURE)
      {
        error ("CODE MODULE NAME - function name - Error string.");
        return FAILURE;
      }

    if (do_something_else () == FAILURE)
      {
        error ("CODE MODULE NAME - function name - Another error string.");
        return FAILURE;
      }

One alternative would be:

      if (do_something ())
        {
          if (do_something_else ())
            {
              ...
              return SUCCESS;
            }
          else
            error ("CODE MODULE NAME - function name - Another error string.");
        }
      else
        error ("CODE MODULE NAME - function name - Error string.");

Personally, I prefer the first approach, as:

1. It keeps the error messages close to their originating functions.

2. It's more compact, and doesn't cause a massive load of indentation.

3. Idiomatically speaking, it represents a linear progression of "Try this; did it fail? If so, stop. Try the next thing... etc... everything done, the procedure was a success."

   However,

   1. It introduces many return paths, which could be considered bad coding practice.

   2. Putting function calls in if statements seems a bit strange. This could be rectified with something like:

      bool_t result;
      result = do_something ();
      if (result == FAILURE)
        {
          error ("CODE MODULE NAME - function name - Another error string.");
          return FAILURE;
        }
      

   but in my opinion this is a bit long-winded.

Use of typedefs

Typedefs of plain data

This is a practice that I'm personally in favour of.
For an example in the current code, consider this extract from map.h:

/* -- TYPEDEFS -- */

typedef uint16_t dimension_t;    /**< Type for tile-based map dimensions. */

typedef uint16_t layer_tag_t;    /**< Type for layer tags. */

typedef int32_t  layer_count_t;  /**< Type large enough to hold a layer count. */
typedef uint16_t layer_index_t;  /**< Type for layer indices. */
typedef uint16_t layer_value_t;  /**< Type for layer value data. */
typedef uint16_t layer_zone_t;   /**< Type for layer zone data. */

typedef int32_t  zone_count_t;   /**< Type large enough to hold a zone count. */
typedef uint16_t zone_index_t;   /**< Type for zone indices. */
typedef uint16_t zone_prop_t;    /**< Type for zone properties bitfields. */

Each typedef assigns a concrete integer type (stdint.h-style types, themselves typedefs) to another type encoding its semantics.

Arguments for:

1. Allows type contexts/semantics to be expressed (for example, layer_index_t should be used for /indexing/ a /layer/, and will always be large enough to hold a layer index; layer_zone_t represents the type of data found in a /layer zone plane/.
2. Allows easy switching of concrete data types later - for example, one could change dimension_t to uint32_t if the maximum map size suddenly changes to 4294967295x4294967295 tiles.

Disadvantages:

1. Hides the concrete data type.
2. Requires that headers containing typedefs be included.

Typedefs of structs

This is somewhat more controversial practice and one that I'm very open to input on.

An example in the current code:

typedef struct map
{
  dimension_t width;                /**< Width of the map, in tiles. */
  dimension_t height;               /**< Height of the map, in tiles. */

  zone_index_t    max_zone_index;   /**< Highest zone index in the map. */
  zone_prop_t    *zone_properties;  /**< Array of zone property bitfields. */

  layer_index_t   max_layer_index;  /**< Highest layer index in the map. */
  layer_tag_t    *layer_tags;       /**< Array of map layer tags. */
  layer_value_t **value_planes;     /**< Pointers to map layer value planes. */
  layer_zone_t  **zone_planes;      /**< Pointers to map layer value planes. */

} map_t;

If struct typedefs were to be allowed, it would likely be a style point that the typedef name and struct name share the same root. That is, the structure with name NAME would be accessible either as "struct NAME" or "NAME_t".

Advantages:

1. Less to type out (NAME_t as opposed to struct NAME).
2. Creates consistency with the other variable type naming conventions.
3. The hiding of the actual type of the variable could be used as a tool for encapsulation, if handled properly (and that's a big if). Moreover, If accessors and mutators were properly created, then the actual data type behind the typedef could be changed from a struct to plain data (for example, an index into an array in another struct) if it is later deemed necessary.

Disadvantages:

1. The aforementioned, with a strong caveat that it blurs the line between standard data and structs, without the use of Hungarian notation which is likely not a good compromise. Perhaps this would be a lesser issue if programmers were encouraged to use accessors and mutators on structs, thus preventing them from needing to know whether variables have members or not.
2. Thwarts forward declaration (which is necessary in many headers), so the use of "struct TYPE" would not be fully eliminated, causing a mixture of struct TYPE and TYPE_t in code which would harm consistency if not properly handled.

Pointer typedefs

My personal opinion is that pointers should /not/ by typedef'd as regular types (eg no typedef struct map* map_ptr_t, as it hides C pointer conventions (eg passing by reference) and would likely lead to Hungarian notation popping up to differentiate between variable types and pointer types. Comments?

Since I'll be away for a week's holiday with a computer but no Internet, and I'm not by any stretch of the imagination the only person working on Crystals, I thought it would be a good idea for us to discuss which areas of the engine we're interested in working on in the near future (to avoid reduplication of effort, and suchlike.)

Items I'm looking to work on soon (some of which will be done over the week) are:

• Rewriting the Crystals map format to be a type of IFF (http://www.martinreddy.net/gfx/2d/IFF.txt) file, as IFF is already quite close to what we're doing for the format, and it might be nice to draw the format closer to existing standards. Given that CMF1 is already split into chunks, it would be nice to be able to read in those chunks in any order and discard any that aren't needed by the implementation. This would block any other changes to mapload.c and the map editor - this would likely be a nice task for me to do over the week as it'll minimally affect most areas.
• Creating a central timer, using glib2, which will then dispatch the graphics updating and input routines in a regular manner. This would block changes to quite a large part of the code (graphics.c, event.c, their SDL counterparts, and main.c to name a few), so I might put this off until I can communicate with others.
• Possibly changing Crystals's internal types to point to their glib2 equivalents, as glib2 can do a better job of portability than we can. This would likely affect just types.h. (Currently on GNU systems types.h includes stdint.h, which might be a better idea when we can depend on stdint.h.)

Pursuant to something barrucadu raised in person, and given that there is such a lack of development interest in the project, I've decided to drop the formal procedure for merging changes.

As of now,

• Staging is deprecated. Its role as a collection of (relatively) stable things people have been working on goes to master, which will immediately be pushed to with the contents of staging as they are at the moment.
• Anyone with access to this repository may pull their work into master when they feel it's stable enough. No need for a pull request.
• Pull requests into this repository no longer need a majority vote, and anyone can merge them.
• Anyone breaking the codebase (by pulling broken code into master, etc) is liable to get moaned at by the other developers, but that's up to said developers. It's no longer an issue of policy to keep master tidy, but out of courtesy to others (developers and users- pah, users!- alike) you should definitely make an effort to do so.
• Issues that affect the project will still be posted up here for discussion, but at the end of the day it's up to developers to reach equilibrium about the consequences.
• No. More. Feature. Freezes.

Apologies to giselher, who I'm aware gets left out of a lot of discussion about the project because of majority consensus being achieved by myself and barrucadu all the time.