Code Change Requests

For discussion of the code running behind the game

Moderator: Staff

User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Code Change Requests

Postby Roots » Fri Feb 05, 2010 8:50 am

This thread is for submitting open requests to the programming team for wide-scale changes to existing code and standards. The previous version of this thread is located here.

Purpose: This thread has the following purposes
  • To propose changes to existing code that would affect more than one major area of the codebase
  • To discuss and come to a decision regarding any aforementioned proposals
  • To work together to keep our code base orderly and consistent
  • To bring the team's attention to issues in existing code such as poor documentation

Use: You should post in this thread for any of the following reasons
  • You want to change the name of a header, source or script file
  • You want to add/remove/rename a directory or move existing files to other directories
  • You want to rename a class, namespace, function, enum, constant, etc. that is used on a wide scale throughout the code
  • You have discovered code that has major issues such as non-compliance with the code standard or inconsistent error handling
  • You have an issue with or proposed change to our code standard
  • You wish to share your approval/disapproval/indifference/issues with any proposals that others make.
Image
User avatar
gorzuate
Developer
Posts: 2575
Joined: Thu Jun 17, 2004 3:03 am
Location: Hermosa Beach, CA
Contact:

Re: Code Change Requests

Postby gorzuate » Sun Feb 07, 2010 5:35 am

Pause mode has two files pause.cpp and pause.h sitting in src/modes.
Same with scene.
That's fine, but save mode has its own directory, and only two files, save_mode.cpp and save_mode.h. Shouldn't they be moved up one directory and be renamed save.h and save.cpp?
Image
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Sun Feb 07, 2010 7:29 am

Well right now save mode is really just skeleton code. The finished save mode that I envision at least allows you to (maybe) name your save game files, shows you a full list of the saved games available, does automatic version checking to make sure the saved game is compatible with the latest release, shows you a preview of the saved game by showing the active party characters, drunes, time played, location, etc., allows you to delete saved games, etc. That's probably going to take more than one file to do all that I think.

I do want to rename save_mode.* to just save.* though. None of our modes have a _mode part on the filename (no map_mode.cpp, no battle_mode.h, etc). That's something that can/should be done right now. I think we'll have to come back to whether or not save mode warrants its own directory once it is fully featured and we can see if its reasonable to fit it all in a single file or not.
Image
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Mon Feb 08, 2010 5:02 am

At some point, I think a good chunk of our existing code needs to be moved into src/common/. Any useful classes, functions, enums, globals, etc. that are not specific to game modes should be going in here. Some of our existing classes that should be moved here might need to be made a little bit lighter/abstract and then have the different modes that use them create derivative classes to allow more flexibility. Here's a list of code I've identified that I feel should be moved to src/common/.

  • MapDialogue class in map_dialogue.h (used in both map and battle code)
  • MessageWindow class in menu_views.h (used in both menu and boot code)
  • ShopObjectViewer in shop.h (used in shop mode, would be damn useful for menu mode to re-use)
  • ObjectListDisplay in shop_utils.h (used in shop mode, useful for menu mode)
  • ObjectCategoryDisplay in shop_utils.h (used in shop mode, useful for menu mode)

This isn't something we have to or should do all at once. We can do it piece by piece on a "when-we-get-around-to-it" kind of basis. Also low priority.
Image
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Tue Feb 23, 2010 3:57 am

Items and skills both have target type members. Particularly they have two members related to identifying the target

Code: Select all

   /** \brief The type of target for the item
   *** Target types include attack points, actors, and parties. This enum type is defined in global_actors.h
   **/
   GLOBAL_TARGET _target_type;

   //! \brief If true the item should target allies, otherwise it should target enemies
   bool _target_ally;


The GLOBAL_TARGET enum has these values:

Code: Select all

/** \name Target Types
*** \brief Enum values used for declaring the type of target of items, skills, and actions.
**/
enum GLOBAL_TARGET {
   GLOBAL_TARGET_INVALID      = -1,
   GLOBAL_TARGET_ATTACK_POINT =  0,
   GLOBAL_TARGET_ACTOR        =  1,
   GLOBAL_TARGET_PARTY        =  2,
   GLOBAL_TARGET_SELF         =  3,
   GLOBAL_TARGET_TOTAL        =  4
};


Its annoying having to grab two pieces of information for identifying the target. I think it was done this way because we want skills/items to target enemies/allies by default, and allow the player the option to select the opposing party to use the item on. Anyway, I want to get read of the _target_ally boolean on items/skills and replace the GLOBAL_TARGET endum with this:


Code: Select all

/** \name Target Types
*** \brief Enum values used for declaring the type of target of items, skills, and actions.
**/
enum GLOBAL_TARGET {
   GLOBAL_TARGET_INVALID      = -1,
   GLOBAL_TARGET_SELF         =  0,
   GLOBAL_TARGET_SELF_POINT   =  1,
   GLOBAL_TARGET_ALLY         =  2,
   GLOBAL_TARGET_ALLY_POINT   =  3,
   GLOBAL_TARGET_ENEMY        =  4,
   GLOBAL_TARGET_ENEMY_POINT  =  5,
   GLOBAL_TARGET_ALL_ALLIES   =  6,
   GLOBAL_TARGET_ALL_ENEMIES  =  7,
   GLOBAL_TARGET_TOTAL        =  8
};


If we later decide we want the ability to allow skills/items intended for allies/enemies to have the option to be used on enemies/allies, we'll figure it out at that time. For now, i say we forget about any such functionality and go with this cleaner solution to targets. Approved?
Image
rujasu
Developer
Posts: 758
Joined: Sun Feb 25, 2007 5:40 am
Location: Maryland, USA

Re: Code Change Requests

Postby rujasu » Tue Feb 23, 2010 4:39 am

That's probably best, anything we can keep simple at this point, do so.
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Sun Feb 28, 2010 11:18 pm

I'll change the target types sometime in the near future. It will be a bit of work because there are dependencies on the current types, so I want to wait a bit until the code is in a stable state to afford such a change.

----------

Something that's been bothering me for the past two weeks are SystemTimers. I designed these a while back and they're really great to use in many circumstances. But one "feature" I created for system timers really seems to be more of an inconvenience. I'm referring to the way that timers are updated when they are running. Updates are done automatically by the SystemEngine class, and game modes have to pause/resume timers if they don't want to update. The problem with this is the main game loop updates the timers before it updates the modes which will pause/resume those timers.

Code: Select all

      while (SystemManager->NotDone()) {
         .....
         // 4) Update timers for correct time-based movement operation
         SystemManager->UpdateTimers();

         // 5) Update the game status
         ModeManager->Update();
      } // while (SystemManager->NotDone())


So lets say map mode has a timer and determines on a given loop that it wants to pause that running timer. Oops! Too late. The timer already updated itself for this loop. And later when map mode wants to resume that timer, it won't actually resume until the next loop. You may say "so what?". Well, this can lead to timing bugs, especially if timers are paused/resumed frequently.

So I think that timers should get an Update() function and should require the user to manual update the timer when and where it is necessary. I actually tried to avoid this in my initial design because I was tired of having to call update functions on just about every little thing and thought I'd be clever and do it differently, but it doesn't seem to be working out. Its especially problematic in battle mode, where under certain circumstances we have to pause or resume a good dozen or more timers.

I still want to leave the option of using the "auto update" feature (some engine code may want to make use of timers and not have to worry about updating them), but that will have to be requested manually and will not be the default setting. All timers not in auto-update mode will have to be updated manually for them to make any progress.



What do you guys think of this proposal? Good? Bad? Could be better? If there's no complaints I'll plan to make this change sometime in the near future.
Image
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Tue Mar 02, 2010 3:20 am

I want to create a new file pair: src/common/global/global_utils.*. There are various enums shared in the existing global code that need to be used in more than one place. For instance, GLOBAL_TARGET needs to be used by skills and attack points and GLOBAL_USE needs to be used by items and skills. Right now global_actors.* contains this code and main global files have to include global_actors.h as a result even though they don't need to use this class. I will also move/create some small global utility functions, like returning a text representation of an enum value.

I'm going to assume that this is fine with everyone and go ahead with it. It will probably be added in a commit tomorrow. If you have an issue to raise, raise it real soon. :angel:
Image
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Wed Mar 03, 2010 12:53 pm

Global target enum change has been committed. I'm going ahead with my proposed SystemTimer changes since no one responded. Its going to be difficult to implement status effects in battle without these changes since some effects require messing with the actor's state timer.
Image
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Thu Mar 04, 2010 9:10 am

SystemTimer changes are now complete. When you create a system timer it is placed in "manual mode" by default and you have to call Update() yourself for the timer to update. I think the implementation is pretty solid and both manual and automatic modes appear to be working well.

-----

Eventually, defs_global.cpp should be renamed to defs_common.cpp and moved from src/common/global/ to src/common/ so we can provide bindings for more than just the global code in the common source. Pretty simple and low priority if someone has a few minutes to spare and wants to do this work. I won't be touching it because I'm busy with other stuff right now.
Image
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Mon Mar 08, 2010 12:30 am

I'm wondering if we can do something to make constants/enums bound to Lua easier to use. Luabind requires that constants/enums be bound to a class rather than just a namespace (has something to do with Lua metatable values). So we've been binding constants and enums to the closest associated manager class. The drawback of this approach is that the name is quite...long.

Code: Select all

user:RegisterStatusChange(hoa_global.GameGlobal.GLOBAL_STATUS_FORTITUDE_BOOST, hoa_global.GameGlobal.GLOBAL_INTENSITY_POS_LESSER);


That's what I had to write for two constants passed as arguments to a function. Yuck. Since we have to use a class to bind these constructs, I was wondering if perhaps we could create a special empty class for this purpose in defs.cpp and bind all constants and enums to it. That's right: all of them. The class could go in the global Lua namespace or perhaps a short namespace like "hoa". Maybe the class could be called "const" or something? So that earlier line may look like so:


Code: Select all

user:RegisterStatusChange(hoa.const.GLOBAL_STATUS_FORTITUDE_BOOST, hoa.const.GLOBAL_INTENSITY_POS_LESSER);



What do you guys think?
Image
User avatar
gorzuate
Developer
Posts: 2575
Joined: Thu Jun 17, 2004 3:03 am
Location: Hermosa Beach, CA
Contact:

Re: Code Change Requests

Postby gorzuate » Mon Mar 08, 2010 10:19 pm

If it really bothers you that much, fine. hoa.const isn't much of an improvement.
Image
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Thu Jul 14, 2011 8:42 pm

After the July release, I'm going to do the const change above unless someone has a good reason to stop me.


Another post-release change i'm going to make is removing src/common/global/global_defs.cpp and replacing it with src/common/common_defs.cpp. The reason being that the global code is a component of the common code, and not the other way around. Some of the common classes require lua bindings and right now that's going in global_defs.cpp, which is...awkward to say the least.
Image
rujasu
Developer
Posts: 758
Joined: Sun Feb 25, 2007 5:40 am
Location: Maryland, USA

Re: Code Change Requests

Postby rujasu » Thu Jul 14, 2011 11:59 pm

Huh, I don't remember seeing this before.

I dunno, this seems kind of hackish to me. It's shorter, but may be harder to follow/understand since we're not using the classes themselves. I guess it's up to you, but I can't say I'm a fan. :shrug:

Also, I wouldn't use "const." Maybe that's fine in Lua, but it looks too much like something that could be a keyword, seeing as it is one in C++. Again, maybe I'm being pedantic there, but I'd try to go for something other than "const."
User avatar
Roots
Dictator
Posts: 8666
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Code Change Requests

Postby Roots » Fri Jul 15, 2011 12:23 am

Yeah I was surprised you never commented on it. I'm not a fan of this either, but I'm even less of a fan of having to attach constants to arbitrary classes that are defined in a namespace, making it extremely verbose to call them.

Good point with avoiding const. Maybe we could instead use "hoa" as our constant class?
Image
rujasu
Developer
Posts: 758
Joined: Sun Feb 25, 2007 5:40 am
Location: Maryland, USA

Re: Code Change Requests

Postby rujasu » Fri Jul 15, 2011 12:55 am

Roots wrote:Good point with avoiding const. Maybe we could instead use "hoa" as our constant class?


Sure, I guess that works.

Return to “Programming”

Who is online

Users browsing this forum: No registered users and 2 guests