Post-Apocalyptic RPG forums

Development => Programming => Topic started by: b0rland on August 05, 2009, 09:48:46 AM



Title: Code review/complaints/discussion
Post by: b0rland on August 05, 2009, 09:48:46 AM
Following Kaydeth's proposal, I'm creating this topic as a place to discuss things to be improved in the code. Those that are generally considered a good idea will afterwards become refactoring tickets in Trac.


Title: Re: Code review/complaints/discussion
Post by: b0rland on August 05, 2009, 10:18:45 AM
First complaint :).
Dialog window ownership in Hud and World.
In most cases Hud object creates and owns dialog windows (hud.py:38-41), although World accesses them as if they were its own (world.py:72-83). In some cases (e.g. Inventory), World itself owns the dialog window (world.py:87-90).
I fail to see the difference in these cases that requires different approach. My suggestion is for hud to own all the dialogs, for world to only interact with them via hud's functions.
Examples of offending usage:
Code:
        self.hud = hud.Hud(self.engine, TDS)
        self.hud.events_to_map["inventoryButton"] = cbwa(self.displayInventory, True)
        self.hud.events_to_map["saveButton"] = self.saveGame
        self.hud.events_to_map["loadButton"] = self.loadGame
        hud_ready_buttons = ["hudReady1", "hudReady2", "hudReady3", "hudReady4"]
        for item in hud_ready_buttons:
            self.hud.events_to_map[item] = cbwa(self.hud.readyAction, item)
        self.hud.hud.mapEvents(self.hud.events_to_map)
        .....
        inv_data = {'A1':'dagger01', 'A3':'dagger01'}
        self.inventory = inventory.Inventory(self.engine, inv_data, self.refreshReadyImages)

Suggested refactoring:
Code:
       self.hud = hud.Hud(self.engine, TDS)
       # Hud will init Inventory too; inv_data should be fetched from some inventory model object, but that's separate story, can stay hardcoded for now
       events_to_map["inventoryButton"] = cbwa(self.displayInventory, True)
       events_to_map["saveButton"] = self.saveGame
       ....
       self.hud.mapHudEvents(events_to_map)
       ....
       self.hud.mapMenuEvents(events_to_map)
       ....
       self.hud.mapInventoryEvents(events_to_map)


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on August 06, 2009, 01:26:17 PM
Makes sense to me as long as you are only talking about the GUI part of the Inventory. It doesn't make sense to me for the actual contents of the inventory to be owned by the Hud class.

What about context menus and container displays? Should these also be a part of the HUD?


Title: Re: Code review/complaints/discussion
Post by: b0rland on August 07, 2009, 09:02:19 AM
Makes sense to me as long as you are only talking about the GUI part of the Inventory. It doesn't make sense to me for the actual contents of the inventory to be owned by the Hud class.
Yes, that was a second proposal I planned to make. Currently both the gui for inventory and the inventory contents storage seem to be done by the same class which is a part of "view". I think Inventory (as in contents storage and management device) will have enough non-trivial logic to move it into separate class inside "model". InventoryGUI should reference and query it when drawing or processing events.

What about context menus and container displays? Should these also be a part of the HUD?
That sounds sane. But I think context menus are implemented on FIFE level and are in a separate hierarchy. That needs some more research.


Title: Re: Code review/complaints/discussion
Post by: b0rland on August 07, 2009, 09:30:16 AM
My bad. Context menu is actually in PARPG, so it needs to be owned by HUD too.


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on August 07, 2009, 01:47:55 PM
I think Inventory (as in contents storage and management device) will have enough non-trivial logic to move it into separate class inside "model".

Agreed except I don't think you mean "model" as that is a FIFE class. We'll work that out once the Inventory is actually finished though.


Title: Re: Code review/complaints/discussion
Post by: b0rland on August 09, 2009, 08:26:53 AM
Right. I'm referring to the model/view separation of PARPG's code (and I mean model/view as design patterns rather then class names). Supposedly the idea was to make world class the entry point to all view code and engine class to represent the model code.


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on August 09, 2009, 07:40:00 PM
Unless there are any objections or alternate suggestions we'll go ahead and create tasks for these changes.

In summary:

We'll move Inventory, Context Menus, and Container displays into the Hud Class.  Displaying any of these will then require access to the Hud class. In the long run this should be GUI components only. Contents of these components should be created and managed elsewhere.


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on August 11, 2009, 10:41:17 PM
FYI, tickets created:

http://parpg-trac.cvsdude.com/parpg/ticket/78
http://parpg-trac.cvsdude.com/parpg/ticket/79
http://parpg-trac.cvsdude.com/parpg/ticket/80


Title: Re: Code review/complaints/discussion
Post by: b0rland on August 12, 2009, 06:51:28 AM
Thanks a lot. I've been reading the code and it seems there's much more than that to untangle. But at least it still seems like a good idea.


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on August 12, 2009, 12:59:42 PM
Thanks a lot. I've been reading the code and it seems there's much more than that to untangle. But at least it still seems like a good idea.

Yep. One step at a time :)


Title: Re: Code review/complaints/discussion
Post by: Cruul on August 14, 2009, 12:49:17 AM
Looked through the code a bit.

Modified the popup.py code to simplify things and created a patch.
Created a ticket in Trac for the patch #81.

-Cruul


Title: Re: Code review/complaints/discussion
Post by: b0rland on August 14, 2009, 09:03:04 AM
Modified the popup.py code to simplify things and created a patch.
Created a ticket in Trac for the patch #81.

Good one. Thanks for the patch. That certainly needed being done.


Title: Re: Code review/complaints/discussion
Post by: b0rland on August 14, 2009, 09:05:33 AM
A riddle.

Can anyone guess how are PARPGFileBrowser and FileBrowser classes different? It's not immediately obvious to me.


Title: Re: Code review/complaints/discussion
Post by: Atomic on August 14, 2009, 09:50:05 AM
On a similar note gui_container.py and popups.py both have a class ContainerGUI that are about 90% the same.  Anyone know which is the "real" ContainerGUI?


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on August 14, 2009, 02:01:51 PM
A riddle.

Can anyone guess how are PARPGFileBrowser and FileBrowser classes different? It's not immediately obvious to me.

They almost look like copies :\. I don't get it either. By the way Python is wierd. How can PARPGFileBrowser inherit from FileBrowser and then just overwrite all the methods? That's just wrong I say :P


Title: Re: Code review/complaints/discussion
Post by: saritor on August 14, 2009, 09:57:23 PM
So today I started looking in detail to the door/teleport code that currently exists to see what is or is not broken.

From what I see there are a couple things going on.
1. There is no object code currently for doors. This should be easy enough to implement on its own however.
2. This is the confusing part. The action code for changing a map seems to be rather jumbled and picking it apart has become rather... odd.

First if we take a look at engine.py we can see that there is code there already to handle the addition of doors to the map, and then we move down to the getItemActions method particularly the if case dealing with Doors we see this.

Code: (engine.py)
            elif obj.trueAttr("Door"):
                actions.append(["Change Map", "Change Map", \
                       self.gameState.PC.approach, [obj.X, obj.Y], \
                        ChangeMapAction(self, self.doors[str(i.ID)].map, [i.destx, i.desty])])
                pass

There are a few problems with in that code that I can see currently as well, just some simple modifications. However if we take a glance as the ChangeMapAction class you will notice that there is some cyclic tendencies to the code.

Code: (action.py)
class ChangeMapAction(Action):
    """A change map scheduled"""
    def __init__(self, engine, targetmap, targetpos):
        """@type engine: Engine reference
           @param engine: A reference to the engine.
           @type targetmap: String
           @param targetmap: Target mapname.
           @type targetpos: Tuple
           @param targetpos: (X, Y) coordinates on the target map.
           @return: None"""
        self.engine = engine
        self.targetpos = targetpos
        self.targetmap = targetmap

    def execute(self):
        """Executes the mapchange."""
        self.engine.changeMap(self.targetmap, self.targetpos)

In the execute method, there is a call being made to self.engine.changeMap() which is (probably obvious by now) a method defined in engine.py.

Can someone explain the reasoning behind that to me? Or is this something that should be consolidated into ChangeMapAction in a better way? It just seems odd...


Sorry to be a pain.

~Saritor


Title: Re: Code review/complaints/discussion
Post by: Atomic on August 14, 2009, 10:36:41 PM
It looks to be a callback function.  If you're familiar with C, it's like passing a function pointer, but in this case, it's doing it in an object-oriented manner.  

I'm still getting familiar with the code myself, so someone correct me if I'm wrong, but here's my take on it:

The method getItemActions() returns all the possible things you can do with that specific item.  The method gets passed an object ID, and returns a list of Action objects.  The Action class is basically a function pointer wrapped in an object.  It's a way of dynamically determining what function should be executed at runtime depending on the situation.  If you look at the options for NPC, you can talk to the NPC or attack it (although right now the callback objects haven't been written and included as parameters).   Similarly for the door, you can do a ChangeMapAction.  When execute() is called on the ChangeMapAction object, it will execute the changeMap() back in the Engine class, but that's not entirely unusual since it looks like Engine is basically the gatekeeper for what goes on as far as the logic is concerned.  If you look at the other execute() methods in actions.py, they all do callbacks to the Engine class.

EDIT:  I realized I didn't completely answer your question.  As to why it's in the Engine class and shouldn't be in the ChangeMapAction class is that the main loop for the game logic is called through Engine.pump() (engine.py, line 298), which in turn calls Engine.handleCommands() (engine.py, line 293), which then checks if the boolean flag self.mapchange has been set to true to indicate that a map change is needed.  This flag gets set by the callback method Engine.changeMap() that gets called in ChangeMapAction.execute(). If the ChangeMapAction class didn't call back to the Engine class and set that flag, then the main loop in pump() wouldn't know if a map change was needed.  It's fairly complex but not out of the ordinary.  This is where UML class interaction diagrams come in really handy ;)





Title: Re: Code review/complaints/discussion
Post by: Atomic on August 15, 2009, 12:59:24 PM
Looked through the code a bit.

Modified the popup.py code to simplify things and created a patch.
Created a ticket in Trac for the patch #81.

-Cruul

The patch looks good to me.  Much more concise.  I applied it on my system, and it worked with no issue.


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on August 15, 2009, 05:00:11 PM
The patch looks good to me.  Much more concise.  I applied it on my system, and it worked with no issue.

Patch commited and ticket closed. Thanks Atomic and Cruul


Title: Re: Code review/complaints/discussion
Post by: saritor on August 16, 2009, 04:13:44 PM
It looks to be a callback function.

Doh! I see that now. I guess I am just not used to looking at Python in a strict OOP way. Thank you for the clarification.


Title: Re: Code review/complaints/discussion
Post by: Cruul on August 16, 2009, 09:31:03 PM
I added temporary debug statements in both popup.py: ContainerGUI class and gui_containers.py: ContainerGUI.

Created ticket in TRAC:
http://parpg-trac.cvsdude.com/parpg/ticket/85#preview

Basically popups.py:ContainerGUI is the one used so the other should be deleted.
I suggest someone else will verify this , best if that someone would have more knowledge of the code and structure to be sure.


Title: Re: Code review/complaints/discussion
Post by: Cruul on August 16, 2009, 09:39:23 PM
Wanted to bring up the subject of logging.

For debugging purposes a logger (like the Python Logging module) would be nice.
This would simplify cleanup of debug-statements for release and isolating log-messages when debugging.

I've been using the logging module from Python with great success in a number of projects where I work.

-Cruul


Title: Re: Code review/complaints/discussion
Post by: mvBarracuda on August 17, 2009, 12:16:06 AM
FIFE comes with logging functionality that surely gets exposed to Python side as well. Therefore we should see if we should rather use the FIFE logger.


Title: Re: Code review/complaints/discussion
Post by: mvBarracuda on August 17, 2009, 01:33:14 PM
New refactoring category at the wiki:
http://wiki.parpg.net/Category:Refactoring

In case there is some confusion where the proper place for discussing refactoring is:
- The forums are the best place to actually discuss possible refactoring
- The wiki should be used to draft actual refactoring proposals that get implemented later

You can add your own proposals to the refactoring category at the wiki by adding the following code to your article:
[[Category:Refactoring]]


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on August 17, 2009, 01:49:35 PM
FIFE comes with logging functionality that surely gets exposed to Python side as well. Therefore we should see if we should rather use the FIFE logger.

I agree. Let's use what we have unless Cruul thinks there is an advantage to using a different logger. I don't know the depth of ability that the FIFE logger has but it seems to do that trick.


Title: Re: Code review/complaints/discussion
Post by: tZee on August 17, 2009, 07:41:15 PM
So today I started looking in detail to the door/teleport code that currently exists to see what is or is not broken.

From what I see there are a couple things going on.
1. There is no object code currently for doors. This should be easy enough to implement on its own however.
2. This is the confusing part. The action code for changing a map seems to be rather jumbled and picking it apart has become rather... odd.

First if we take a look at engine.py we can see that there is code there already to handle the addition of doors to the map, and then we move down to the getItemActions method particularly the if case dealing with Doors we see this.

Code: (engine.py)
            elif obj.trueAttr("Door"):
                actions.append(["Change Map", "Change Map", \
                       self.gameState.PC.approach, [obj.X, obj.Y], \
                        ChangeMapAction(self, self.doors[str(i.ID)].map, [i.destx, i.desty])])
                pass

There are a few problems with in that code that I can see currently as well, just some simple modifications. However if we take a glance as the ChangeMapAction class you will notice that there is some cyclic tendencies to the code.

Code: (action.py)
class ChangeMapAction(Action):
    """A change map scheduled"""
    def __init__(self, engine, targetmap, targetpos):
        """@type engine: Engine reference
           @param engine: A reference to the engine.
           @type targetmap: String
           @param targetmap: Target mapname.
           @type targetpos: Tuple
           @param targetpos: (X, Y) coordinates on the target map.
           @return: None"""
        self.engine = engine
        self.targetpos = targetpos
        self.targetmap = targetmap

    def execute(self):
        """Executes the mapchange."""
        self.engine.changeMap(self.targetmap, self.targetpos)

In the execute method, there is a call being made to self.engine.changeMap() which is (probably obvious by now) a method defined in engine.py.

Can someone explain the reasoning behind that to me? Or is this something that should be consolidated into ChangeMapAction in a better way? It just seems odd...


Sorry to be a pain.

~Saritor

I introduced that for two reasons:

1. Maximinus - then the lead programmer - thought it going over-the-top implementing an action stack if there was only 1 action.
2. Due to the ownerships you can't trigger a mapchange from within an actor:

The engine owns the map data. The map owns the layers. In the layers there are the FIFE-Actors and our action listeners registered. If we call a mapchange from within such a listener we would cause the map to be cleared and reloaded, thus killing the owner of our current code location. We would kill the stack below our feet, so to say. :D That's why the mapchange has to be registered and executed when the stack returned to a level above the map.
That's something that could be resolved by having the map class separated and by exchanging the active map, without killing/unloading the current one.


Title: Re: Code review/complaints/discussion
Post by: Bretzel13 on August 17, 2009, 10:14:20 PM
A riddle.

Can anyone guess how are PARPGFileBrowser and FileBrowser classes different? It's not immediately obvious to me.

PARPGFileBrowser inherits FileBrowser, but adds some functionality that forces you to save as a .dat and will give you an error if you don't.


Title: Re: Code review/complaints/discussion
Post by: b0rland on August 18, 2009, 09:04:48 AM
Thank you. That explains. In that case it can probably be shortened quite a bit. http://parpg-trac.cvsdude.com/parpg/ticket/86 -- good task for newcomers.


Title: Re: Code review/complaints/discussion
Post by: saritor on August 18, 2009, 02:00:19 PM
Thank you. That explains. In that case it can probably be shortened quite a bit. http://parpg-trac.cvsdude.com/parpg/ticket/86 -- good task for newcomers.

I'm new!

I posted a patch for others to take a look at
http://parpg-trac.cvsdude.com/parpg/attachment/ticket/86/parpgfilebrowser.2.patch

It appears to me that this patch may be something that would want to be sent upstream to FIFE unless it was the intention of FIFE to allow you to make save files in a non .dat format.


Title: Re: Code review/complaints/discussion
Post by: tZee on August 18, 2009, 03:19:48 PM
I don't see why they would restrict you to .dat files. I personally found it very irritating and annoying that I had to stick to .dat files and that the save files were not handled as .dat files automatically, when I tested the function. :X

Patch looks good to me, will apply it when I get home. (If no one else is faster. :) )

Edit: Patch applied. Thanks. :)


Title: Re: Code review/complaints/discussion
Post by: superfluid on August 22, 2009, 08:58:09 PM
I broke one of my patches up into two to make it easier to cherry pick. They're all at http://2wycked.net/parpg-patches.

I also added another patch to bugfix the toggle renderer keypress handling.

Update: added a patch to fix broken help screen
Update 2: moved inventory code from World to Hud and Inventory


Title: Re: Code review/complaints/discussion
Post by: saritor on September 02, 2009, 09:36:49 PM
In regards to ticket #66 (http://parpg-trac.cvsdude.com/parpg/ticket/66):

I have put together a patch to introduce a door object to the current game. Right now about the only thing it does is stick the door on the map and allow you to view the examine window (check the attached screenshot to view the location of the door, it should be rather obvious :)). Still trying to sort out the actual teleportation aspect of the design since the old stuff seems to somewhat be shrouded in mystery (meaning that MapDoor is called in engine.py but there is no definition for a MapDoor object anywhere and I am slightly confused how it should related with actions and the door object itself).

Anywho, I am not posting the patch to trac right away because I would prefer to just include the teleportation code in a final patch and submit it, but I wanted people to take a look at what I had written so for and made sure I was on the right path in regards to new object creation. Instead the patch should be here as an attachment as well for viewing pleasure.

Thanks,
Saritor


Title: Re: Code review/complaints/discussion
Post by: saritor on September 17, 2009, 09:28:35 PM
This post is mostly for superfluid, but anyone else who is interested take a look.

I have implemented a door on the shanty that when performing the "Change Map" option on the door it will teleport you to a new location on the map. This is the beginnings of what doors will look like, though I am working on a method for automating the process so that travel between maps does not require you to pop up a context menu, instead you will be able to just walk across a tile or two and it should transport you to the next map.

Patch is diffed against a current svn snapshot.

EDIT: Also if you feel like playing with the place the PC gets teleported to, just crack open <PARPG>/maps/map.xml and search for the following lines

Code:
<!-- A sample door -->
<i x="-2.0" o="shanty-door" z="0.0" y="6.0" r="0" id="shanty-door01" object_type="ShantyDoor" is_open="False"
name="Shanty Door" text="Looks like the entrance to the building." target_map_name="my-map" target_map="maps/map.xml"
target_x="10.0" target_y="10.0"></i>

Then change the target_x and target_y values around some.


Title: Re: Code review/complaints/discussion
Post by: amo-ej1 on October 03, 2009, 07:07:40 PM
Saritor, i had a small look on the load/save which fails (http://parpg-trac.cvsdude.com/parpg/ticket/73 ), I commited a small patch which has some overlap with the patch you added here (I thought your teleport code was already checked in), I added basicly support for storing the map name in the gamestate and the map will only be loaded when it is not yet in the game cache.



Title: Re: Code review/complaints/discussion
Post by: saritor on October 09, 2009, 04:53:16 PM
Saritor, i had a small look on the load/save which fails (http://parpg-trac.cvsdude.com/parpg/ticket/73 ), I commited a small patch which has some overlap with the patch you added here (I thought your teleport code was already checked in), I added basicly support for storing the map name in the gamestate and the map will only be loaded when it is not yet in the game cache.



I posted a patch to get teleporters and door objects in there.

http://parpg-trac.cvsdude.com/parpg/attachment/ticket/66/teleport_player.patch


Title: Re: Code review/complaints/discussion
Post by: saritor on October 10, 2009, 10:25:44 PM
So after much discussion with Kaydeth and amo-ej1 we realized a couple things as of the teleport patch I submitted.

1) Teleporting between maps won't work because loading maps (after the first one has been loaded) is not working.
2) Loading maps is not working because of a couple reasons. One of them is that there are conflicts with object id's not being unique enough when changing maps. (ie, an object can not currently have the same id as another even if its on a different map due to the caching method) We decided to address that by making the Gamestate.objects hash a double hash of map id and object id. The other issue is more or less dealing with FIFE problems that haven't been 100% worked out yet as of this post.

Since they seem to be separate entities I went ahead and added the double hash to gamestate in this patch: http://parpg-trac.cvsdude.com/parpg/attachment/ticket/66/gamestate_double_hash.2.patch


Title: Re: Code review/complaints/discussion
Post by: amo-ej1 on October 11, 2009, 11:38:08 AM
I merged the double hash patch, also modified map.xml  and map2.xml to make traveling back and forth possible. At this point the 'new' map gets loaded properly (and seems to work). At this point there still is some camera related issues when travelling from map to map2 to map. Which still needs to be taken a look at.


Title: Re: Code review/complaints/discussion
Post by: Kaydeth on October 11, 2009, 03:12:42 PM
I think we still need to do some work with moving the PC from map to map.

I would like to be able to remove the PC entry from all but the starting map. This will let us modify the PC sprite very easily. In order to do that we need to be able to automatically generate the FIFE instance required on each map.


Title: Re: Code review/complaints/discussion
Post by: maximinus on January 22, 2010, 02:04:18 PM
Old thread, I'm just reviving because there's a few things I like to fix.

First off the bat:

/scripts/common/utils.py should just be rolled in run.py

It's 4 lines of code that we'll almost certainly never touch again.