ObjectController Cleanup: Plan of Action

A place to discuss and present upcoming features for the SWG:ANH Server

ObjectController Cleanup: Plan of Action

Postby apathy » June 12th, 2010, 11:42 am

Background

The ObjectController is one of the most pervasive pieces of code in our core. Nearly every bit of code that manages gameplay features is found in this one class. Think on that for just a moment: every single action that a player can perform for every single profession is encased inside this one solitary class. To make matters worse, a unique instance of this ObjectController class is required for every Object instance. All of this leads to a laundry list of issues that ultimately makes the code brittle and a nightmare to work with:

1) Any changes in the ObjectController header, or in any of the header files in the Object hierarchy results in a nearly complete recompile of the server. This wastes a significant amount of developing time.

2) A significant amount of memory overhead is required to maintain a 1 to 1 status of ObjectController to Object. This is mostly unnecessary overhead as in reality only a single instance of the ObjectController needs to exist.

3) Circular references are "evil." The ObjectController, Object and MessageLib classes are all entwined in the ugliest ménage à trois of code found in the server. This can cause issues when making changes to code that can have unseen (and unwanted) effects due to this tight coupling. It's also another contributor to the first issue listed above.

Plan of Action

In order to properly deal with this situation ObjectController as it exists now must be dismantled and organized into smaller, logically grouped sections of code. At the same time in order to avoid disruption of the ongoing work of others the transition must be planned to have minimal impact both to existing code and to the in-progress work of other developers. In other words one of the most important goals to maintain throughout this change is to keep the server fully functional at every commit.

Below outlines a 5 phase plan of action to deal with the ObjectController class and address the issues outlined in the "Background" section above.


Phase 1: Replace c-style function pointers with std::function usage

This gives us a greater degree of flexibility moving forward for handling callbacks over plain c-style function pointers. In addition, these new style of handlers will return a bool to indicate whether or not the handler completed successfully. This can be used for common ObjectController tasks such as determining when to deduct HAM costs.

Once this stage is complete the server will still be fully operational, all changes are hidden behind the scenes.

Phase 2: Create a second command map to hold rewritten handlers

Starting on line 366 of ObjectController.cpp (commit hash: 6a54867d11) the command map is searched for the appropriate handler for a given message and then that handler is invoked to process the message. A second command map to handle rewritten handlers should be added and checked first, defaulting back to the old command map only if a "new-style" message handler is not found.

This adds some additional code to ObjectController to allow seamlessly transitioning the handlers one at a time, however, once the transition is complete the old command map handling can be easily removed.

Again, at the end of this stage the server will be fully operational with only some behind the scenes code being added.

Phase 3: Transition a command map handler to a non-class function

This begins the heart of the effort, which is to remove all unnecessary code from ObjectController. A single handler will be moved out to it's own non-class function and then registered to the new command map outlined in phase 2. Initially, only low-impact handlers should be selected in order to get an idea of how the transition process will work.

While transitioning handlers from the old format to the new one, tasks common to all handlers (such as deduction of HAM costs) are to be removed. This will further simply the responsibilities of each handler resulting in less copy and pasting of code.

At the end of this phase the server will still be fully operational, however, this marks the beginning of a breakage with the old "api" for handling messages. All future handlers created should begin following this new format.

Repeat phase 3 until all handlers have been removed from ObjectController.

Phase 4: Move message queuing responsibilities out of ObjectController

Currently ObjectController is responsible for maintaining the queue of messages to be processed for an Object. This is the last obstacle to overcome which requires each Object to maintain it's own separate instance of ObjectController. Message queuing responsibilities will be transitioned to the Object class itself, either directly by making the message queue a part of the Object's interface or by creating a class specifically for managing queues (separation of concerns). The second method is preferred.

Phase 5: One instance of ObjectController for all

At this point ObjectController will be nothing more than a simple router for Object messages and will be entirely devoid of Object specific data. This means that there will no reason for each object to maintain it's own instance of ObjectController, only a single instance will be needed that all Objects can hold a reference too. This also has a secondary benefit of eliminating all global "helper" classes used by ObjectController.

Fin
Last edited by apathy on June 13th, 2010, 10:10 pm, edited 2 times in total.
Image
~ May the Source be with you ~
apathy
Retired SWG:ANH Staff
 
Posts: 57
Joined: July 16th, 2009, 12:36 am
SWG Official Server: Naritus

Re: ObjectController Cleanup: Plan of Action

Postby apathy » June 12th, 2010, 8:50 pm

I have successfully implemented phases 1 and 2 and have completed the first iteration of phase 3 by moving HandleMoveFurniture out of the ObjectController. This work is available on my personal fork in the feature-oc_cleanup branch.

Just my initial overview of things this process for most handlers should be very straightforward and simple to implement, we'll have this ObjectController class cut down to size in no time!
Image
~ May the Source be with you ~
apathy
Retired SWG:ANH Staff
 
Posts: 57
Joined: July 16th, 2009, 12:36 am
SWG Official Server: Naritus

Re: ObjectController Cleanup: Plan of Action

Postby Ravenlock » June 12th, 2010, 11:35 pm

Haven't read through all this yet but I love you.

*EDIT: I've read through this now and I stand by my original statement.
Ravenlock
 
Posts: 8
Joined: April 19th, 2010, 10:42 am
SWG Official Server: Ahazi

Re: ObjectController Cleanup: Plan of Action

Postby Kronos » June 13th, 2010, 12:40 am

This is very cool. I'm happy to see this coming to fruition and quickly!
Kronos
Retired SWG:ANH Staff
 
Posts: 108
Joined: May 21st, 2010, 11:53 pm
Location: North Idaho
SWG Official Server: Wanderhome

Re: ObjectController Cleanup: Plan of Action

Postby Ravenlock » June 13th, 2010, 8:04 pm

As an extension to this, we're going to make a few more changes.

Handlers will be required to return a bool, indicating whether they completed successfully or not. This will allow us to handle situations like state checks, HAM reductions, etc.. in the ObjectController queue appropriately. I would imagine that, whenever written, the CombatController will benefit from the same notification.

Also, as it stands now, any skill used that reduces HAM is handled inside the <skill>Manager itself, and may or may not be checking the DB for proper values. We'll be fixing that (the methods are already in place, just unused...), and therefore EVERY handler that does this manually will need to remove that. This is important, since we want the config values to come from the DB and *not* be hardcoded.

*EDIT: Ap, please update OP to include that... and perhaps be more eloquent than I? :roll:
...
Ravenlock
 
Posts: 8
Joined: April 19th, 2010, 10:42 am
SWG Official Server: Ahazi

Re: ObjectController Cleanup: Plan of Action

Postby apathy » June 13th, 2010, 10:08 pm

I edited the text in Phases 1 and 3 to reflect our discussions.
Image
~ May the Source be with you ~
apathy
Retired SWG:ANH Staff
 
Posts: 57
Joined: July 16th, 2009, 12:36 am
SWG Official Server: Naritus

Re: ObjectController Cleanup: Plan of Action

Postby Kronos » June 13th, 2010, 11:40 pm

I have a question around the error messages that will be going out to the players if for instance the player doesn't have enough HAM or isn't in the correct state, how are we going to ensure that the player is sent the correct message, instead of a generic one like: you cannot do that in your current state?

This is really the only thing that worries me about doing the check in the oC itself.
Kronos
Retired SWG:ANH Staff
 
Posts: 108
Joined: May 21st, 2010, 11:53 pm
Location: North Idaho
SWG Official Server: Wanderhome

Re: ObjectController Cleanup: Plan of Action

Postby Ravenlock » June 14th, 2010, 1:09 am

A couple things;
1) There's a cbt_spam column in the DB that could help in some cases. But I understand your concern, and we'll need to discuss how best to deal with it.

2) I think we should add a column to the command_table that is a simple boolean indicating whether or not the OC should proceed with these 'default' actions/checks. That way, if a certain command is particularly troublesome, we can slap it around a bit and not have to listen to what the OC says... :twisted:
Ravenlock
 
Posts: 8
Joined: April 19th, 2010, 10:42 am
SWG Official Server: Ahazi

Re: ObjectController Cleanup: Plan of Action

Postby Lloydyboy » June 14th, 2010, 3:15 am

Kronos wrote:I have a question around the error messages that will be going out to the players if for instance the player doesn't have enough HAM or isn't in the correct state, how are we going to ensure that the player is sent the correct message, instead of a generic one like: you cannot do that in your current state?

This is really the only thing that worries me about doing the check in the oC itself.


We could build up some message maps for different states to send based on the opcode. That we we just look for the value in the relevant map (and show a default if there isn't one).

This could be done in the DB table too, and dynamically loaded on startup.
Euro-Chimaera Pre & Post-CU
Lloyd Pickering - Jedi (Post 9 Village)
Zoxara BE, 12pt Chef, 14pt Artisan, Manager of EMP Mall, Commerce City
Lloydyboy
Retired SWG:ANH Staff
 
Posts: 30
Joined: September 7th, 2008, 4:33 am
SWG Official Server: Chimaera


Return to Feature Proposals

Who is online

Users browsing this forum: No registered users and 1 guest

cron