Welcome, Guest. Please login or register.

Pages: 1 [2] 3
Print
Author Topic: Code review/complaints/discussion  (Read 20361 times)
Kaydeth
Community member

Posts: 185



View Profile Email
« Reply #15 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 Tongue
Logged
saritor
Guest
« Reply #16 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
Logged
Atomic
Community member

Posts: 31



View Profile
« Reply #17 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 Wink



« Last Edit: August 14, 2009, 11:17:14 PM by Atomic » Logged
Atomic
Community member

Posts: 31



View Profile
« Reply #18 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.
« Last Edit: August 15, 2009, 02:24:28 PM by Atomic » Logged
Kaydeth
Community member

Posts: 185



View Profile Email
« Reply #19 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
Logged
saritor
Guest
« Reply #20 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.
Logged
Cruul
Guest
« Reply #21 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.
Logged
Cruul
Guest
« Reply #22 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
Logged
mvBarracuda
Admin
Community member

Posts: 1308



View Profile Email
« Reply #23 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.
Logged
mvBarracuda
Admin
Community member

Posts: 1308



View Profile Email
« Reply #24 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]]
Logged
Kaydeth
Community member

Posts: 185



View Profile Email
« Reply #25 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.
Logged
tZee
Community member

Posts: 190


View Profile Email
« Reply #26 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. Cheesy 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.
Logged

Bretzel13
Community member

Posts: 73



View Profile Email
« Reply #27 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.
Logged
b0rland
Community member

Posts: 105



View Profile Email
« Reply #28 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.
Logged
saritor
Guest
« Reply #29 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.
« Last Edit: August 18, 2009, 02:14:06 PM by Saritor » Logged
Pages: 1 [2] 3
Print
Jump to: