Author Topic: Code review  (Read 29572 times)

0 Members and 2 Guests are viewing this topic.

Offline rudistoned

  • Full Member
  • ***
  • Posts: 229
Code review
« on: June 15, 2013, 11:50:04 AM »
Hey guys, I thought we could start a thread to discuss changes to the code (if we think they need discussing).

I noticed that the current revision of master (6cc3e2d77e1e38cd327d244b6c2f19c7d138a770) throws the following error when trying to load image files on windows:

Quote
I'm sorry, but an uncaught exception occurred.

While running game code:
  File "game/library/screens/pyt - screens - nextday.rpy", line 6, in script
  File "game/library/screens/pyt - screens - nextday.rpy", line 130, in python
  File "game/library/classes - characters.rpy", line 668, in python
  File "game/library/functions.rpy", line 148, in python
Exception: Backslash in filename, use '/' instead: u'D:\\PyTFall\\renpy-6.15.4-sdk\\pytfall/game\\content/chars/naruto/Sakura/strip (1).png'

Revision a9e216809559bd2a7f98906cef19762116e0e7b7 (the one the "tags" branch is based on) does not have this problem, so it was probably introduced since.

If one of you feels like fixing it, the following function might come in handy. However, once we use the TagDatabase to get paths to image files, the image loading code will be changed anyway, so you could just ignore it for now.
Code: [Select]
    def normalize_path(path, start=""):
        '''Returns a filesystem path following the conventions of the local OS.
       
        If start is empty, an absolute path will be returned.
        If start is not empty, a path relative to start will be returned.
        '''
        path = os.path.normpath(path)
        # determine an absolute version of path
        if os.path.isabs(path):
            abspath = path
        else:
            abspath = os.path.normpath(os.path.join(gamedir, path))
        # return the normalized path
        if start=="":
            normpath = abspath
        else:
            startpath = os.path.normpath(start)
            normpath = os.path.relpath(abspath, start=startpath)
        return normpath

Offline Xela

  • Global Moderator
  • *****
  • Posts: 6893
  • "It's like hunting cows"
Re: Code review
« Reply #1 on: June 15, 2013, 11:53:09 AM »
Hey guys, I thought we could start a thread to discuss changes to the code (if we think they need discussing).

I noticed that the current revision of master (6cc3e2d77e1e38cd327d244b6c2f19c7d138a770) throws the following error when trying to load image files on windows:

Revision a9e216809559bd2a7f98906cef19762116e0e7b7 (the one the "tags" branch is based on) does not have this problem, so it was probably introduced since.

If one of you feels like fixing it, the following function might come in handy. However, once we use the TagDatabase to get paths to image files, the image loading code will be changed anyway, so you could just ignore it for now.
Code: [Select]
    def normalize_path(path, start=""):
        '''Returns a filesystem path following the conventions of the local OS.
       
        If start is empty, an absolute path will be returned.
        If start is not empty, a path relative to start will be returned.
        '''
        path = os.path.normpath(path)
        # determine an absolute version of path
        if os.path.isabs(path):
            abspath = path
        else:
            abspath = os.path.normpath(os.path.join(gamedir, path))
        # return the normalized path
        if start=="":
            normpath = abspath
        else:
            startpath = os.path.normpath(start)
            normpath = os.path.relpath(abspath, start=startpath)
        return normpath

I've just pushed and a fix for the windows path should be there as well.
Like what we're doing?

Offline rudistoned

  • Full Member
  • ***
  • Posts: 229
Re: Code review
« Reply #2 on: June 20, 2013, 04:39:24 PM »
Code: [Select]
max_page = len(content) / page_size if len(content) % page_size not in [0, page_size] else (len(content) - 1) / page_size

I've never seen this syntax before. Am I correct in assuming that "len(content) / page_size" is assigned to "max_page" if "len(content) % page_size not in [0, page_size]" and if not "(len(content) - 1) / page_size"  is assigned to "max_page"? If so, why not write it like this:
Code: [Select]
if len(content) % page_size not in [0, page_size]:
    max_page = len(content) / page_size
else:
    max_page = len(content) - 1) / page_size

Offline Xela

  • Global Moderator
  • *****
  • Posts: 6893
  • "It's like hunting cows"
Re: Code review
« Reply #3 on: June 20, 2013, 05:40:32 PM »
Code: [Select]
max_page = len(content) / page_size if len(content) % page_size not in [0, page_size] else (len(content) - 1) / page_size

I've never seen this syntax before. Am I correct in assuming that "len(content) / page_size" is assigned to "max_page" if "len(content) % page_size not in [0, page_size]" and if not "(len(content) - 1) / page_size"  is assigned to "max_page"? If so, why not write it like this:
Code: [Select]
if len(content) % page_size not in [0, page_size]:
    max_page = len(content) / page_size
else:
    max_page = len(content) - 1) / page_size

Feel free to swap, either way works :)
Like what we're doing?

Offline Xela

  • Global Moderator
  • *****
  • Posts: 6893
  • "It's like hunting cows"
Re: Code review
« Reply #4 on: June 22, 2013, 03:52:33 PM »
@ Rudi/Matt

Take a look at this function:

Code: [Select]
    # Throwing dice. value = chance, max - limits
    # if dice(60): means probability of 60%
    def dice(value, max=100, show=True):
        number = random.randrange(0,max+1)
        if number <= value:
            result = True
        else:
            result = False
        if config.developer and show:
            notify(u"Resulted in %d from %d, limit - %d, and result - %s." % (number,max,value,str(result)))
        return result

This is from Alkion (written by Roman, not me) but this seems flawed because it actually picks from 101 numbers and not 100. Can any of you confirm this, I ran this on pure python 2.7 and it seems to agree with me.

Code should be:

Code: [Select]
    # Throwing dice. value = chance, max - limits
    # if dice(60): means probability of 60%
    def dice(value, max=100, show=True):
        number = random.randrange(1, max+1)
        if number <= value:
            result = True
        else:
            result = False
        if config.developer and show:
            notify(u"Resulted in %d from %d, limit - %d, and result - %s." % (number,max,value,str(result)))
        return result

Otherwise dice(1) actually means 2% chance because number variable can be anything in range of 0 to 100 (BUT INCLUDING 0 as well)  and same counts for any other throw? What do you think?
« Last Edit: June 22, 2013, 03:57:07 PM by Xela »
Like what we're doing?

Offline rudistoned

  • Full Member
  • ***
  • Posts: 229
Re: Code review
« Reply #5 on: June 23, 2013, 04:15:57 AM »
You are correct. Number should be anything between 1 and 100. That way, 0 will be False, 1 will have 1% chance for True, 99 will have 99% for True and 100 will always be True.

I would prefer to write the function this way:
Code: [Select]
def throw_dice(value, limit=100, show=True):
        '''Returns True if value is <= a random integer between 1 and limit.

        If limit is 100, this means that a value of 60 has a 60% chance for True
        '''
        number = random.randint(1, limit)
        if number <= value:
            result = True
        else:
            result = False
        if config.developer and show:
            notify(u"Resulted in %d from %d, limit - %d, and result - %s." % (number, limit, value, result))
        return result
randint is simpler than randrange and does exactly what we want. limit is a better keyword than max because it does not overwrite a stdlib function name. rolldice or roll_dice are better function names as those names represent the activity of the function better. Documentation on how to use the function should be in the docstring rather than in comments.


Offline Xela

  • Global Moderator
  • *****
  • Posts: 6893
  • "It's like hunting cows"
Re: Code review
« Reply #6 on: June 23, 2013, 04:52:11 AM »
You are correct. Number should be anything between 1 and 100. That way, 0 will be False, 1 will have 1% chance for True, 99 will have 99% for True and 100 will always be True.

I would prefer to write the function this way:
Code: [Select]
def throw_dice(value, limit=100, show=True):
        '''Returns True if value is <= a random integer between 1 and limit.

        If limit is 100, this means that a value of 60 has a 60% chance for True
        '''
        number = random.randint(1, limit)
        if number <= value:
            result = True
        else:
            result = False
        if config.developer and show:
            notify(u"Resulted in %d from %d, limit - %d, and result - %s." % (number, limit, value, result))
        return result
randint is simpler than randrange and does exactly what we want. limit is a better keyword than max because it does not overwrite a stdlib function name. rolldice or roll_dice are better function names as those names represent the activity of the function better. Documentation on how to use the function should be in the docstring rather than in comments.

Great, thanks for your input, I like the name, just don't like the range error.
Like what we're doing?

Offline mijh

  • Newbie
  • *
  • Posts: 11
Re: Code review
« Reply #7 on: June 23, 2013, 09:51:16 AM »
I was going to rewrite and rename the dice function to something like this:
Code: [Select]
def chance(value):
    """Randomly generated percentage chance to return True"""
    return (value / 100.0) > random.random()

Given that the extra arguments are NEVER used anywhere, that's all it needs to be.

Neither does a chance out of 100 have anything to do with dice...

Offline Xela

  • Global Moderator
  • *****
  • Posts: 6893
  • "It's like hunting cows"
Re: Code review
« Reply #8 on: June 23, 2013, 09:53:22 AM »
I was going to rewrite and rename the dice function to something like this:
Code: [Select]
def chance(value):
    """Randomly generated percentage chance to return True"""
    return (value / 100.0) > random.random()

Given that the extra arguments are NEVER used anywhere, that's all it needs to be.

Neither does a chance out of 100 have anything to do with dice...

Hey, actually it can be useful if you want to throw out of 1k (very small chance). Leave it as it is now.
Like what we're doing?

Offline mijh

  • Newbie
  • *
  • Posts: 11
Re: Code review
« Reply #9 on: June 23, 2013, 09:56:59 AM »
Like....
Code: [Select]
chance(0.1)?

A dice() function should return the number of a dice throw, not True or False, anyway. People don't know how to name functions :S
« Last Edit: June 23, 2013, 09:59:45 AM by mijh »

Offline Xela

  • Global Moderator
  • *****
  • Posts: 6893
  • "It's like hunting cows"
Re: Code review
« Reply #10 on: June 23, 2013, 10:04:55 AM »
Like....
Code: [Select]
chance(0.1)?

A dice() function should return the number of a dice throw, not True or False, anyway. People don't know how to name functions :S

LoL

I just don't want to get used to writing chance instead of dice (and dice is shorter  :P )
Like what we're doing?

Offline rudistoned

  • Full Member
  • ***
  • Posts: 229
Re: Code review
« Reply #11 on: June 23, 2013, 10:08:07 AM »
Neither does a chance out of 100 have anything to do with dice...
It seems you never played a pen&paper RPG (or maybe only those without percentage rolls?)  ;)

Offline rudistoned

  • Full Member
  • ***
  • Posts: 229
Re: Code review
« Reply #12 on: June 26, 2013, 08:23:06 AM »
As you prolly have figured out by now, game is coded this way:

1) All logical game elements are coded in pure python. We access, display and change those in RenPy labels.

2) User interactions:

- We jump to a RenPy 'label' (labels are RenPy's way of running the game, instead of the usual main game loop you'd expect to find in Hentai Sims, we simply jump labels until we run out of them (we can obviously keep jumping to the same label any amount of times))
- In that label, we set up background and call screens (screens are coded in RenPy's screen language), at the same time we lock game's flow in a while loop.
There is a number of things that are not smart to include in screens, mainly because screens are being refreshed on all user interactions, once every 20 secs (If I recall correctly) or even many times per second if game requires them to. Another thing is that certain functions (like action calls from buttons) are not searching for variables on the same namespace as the screen first (very unusual for Python).

Problem was here:
Code: [Select]
                        for brtl in hero.brothels:
                            if brtl.highlighted:
                                $imgType = im.Scale
                            else:
                                $imgType = lambda *args: im.Sepia(im.Scale(*args))
                            vbox:
                                null height 20
                                text(u'{size=-10}%s' % brtl.name) align(0.5,0.5)
                                use rtt_lightbutton(img = imgType(brtl.img, 240, 150), return_value = ['brothel', brtl], tooltip = 'Send %s off to work in %s and choose from available actions.' % (chr.name, brtl.name))

In precious revision brothel was used to iterate instead of brtl
Code: [Select]
for brothel in hero.brothels: and at the same time we also had brothel variable in the label and while loop namespaces:

Code: [Select]
label girl_assign:
    scene bg wood
    hide screen girlProfile
    show screen pyt_girl_assign

    python:
       
        if isinstance(chr.location, Brothel):
            brothel = chr.location
            brothel.highlighted = True
            brothel_selected = True
        else:
            brothel = None
            brothel_selected = False

        while True:
            result = ui.interact()

            if result[0] == 'brothel':
                for brothel in hero.brothels:
                    brothel.highlighted = False

                brothel = result[1]
                brothel.highlighted = True
                brothel_selected = True

so this bit of old code:
Code: [Select]
return_value = ['brothel', brothel] didn't actually return a brothel that came from iterating over hero.brothels in the screen (something any Python coder would expect, it actually works exactly like you'd expect something in Python to work since rtt_lightbutton itself is defined on the same namespace as label (it is in fact for all intent and purposes a separate screen itself), but it is still a bit confusing) but instead returned whatever brothel variable was set to in the label. As you can imagine, this is in no way a problem with one brothel but a rather nasty issue with multiple brothels that doesn't throw an Error making it very hard to catch.

In any case this is what I believe was wrong with the code

Well, I just checked because it would be horrifying if renpy would do something like that. Luckily for us, I can not confirm what you say. The change from "brothel" to "brtl" in the for loop (commited in c11e60d...) does not change which brothel is returned by "return_value = ['brothel', brothel]".
How did I check this? I added the following lines to the girlassign screen:
Code: [Select]
------------- game/library/screens/pyt - screens - girlassign.rpy -------------
index ef6f4d6..3b4b973 100644
@@ -4,7 +4,7 @@ label girl_assign:
     show screen pyt_girl_assign
 
     python:
-
+        devlog.info("label girl_assign")
         if isinstance(chr.location, Brothel):
             brothel = chr.location
         else:
@@ -17,9 +17,11 @@ label girl_assign:
 
             if result[0] == 'brothel':
                 for brothel in hero.brothels:
+                    devlog.info("brothel at %i: %s" % (hero.brothels.index(brothel), brothel.name))
                     brothel.highlighted = False
 
                 brothel = result[1]
+                devlog.info("brothel returned: %s" % hero.brothels.index(brothel))
                 brothel.highlighted = True
                 brothel_selected = True


Then I launched the game, bought some buildings and assigned girls in a pattern I remembered. The entries in the devlog show that the return value was always the brothel I had clicked on.

I tried this for both c11e60d (the commit with btrl in the for loop) and its parent c4ba128 (the commit with brothel in the for loop) and got identical results.

Am I still misunderstanding the problem?
« Last Edit: June 26, 2013, 08:25:50 AM by rudistoned »

Offline Xela

  • Global Moderator
  • *****
  • Posts: 6893
  • "It's like hunting cows"
Re: Code review
« Reply #13 on: June 26, 2013, 08:44:08 AM »
Am I still misunderstanding the problem?

In any case this is what I believe was wrong with the code

Well, apparently my believe was False... but I am not spending another minute figuring out that issue, I am having trouble focusing on Guard code as it is. If you want to keep digging at what the hell goes wrong there, be my guest :)
Like what we're doing?

Offline rudistoned

  • Full Member
  • ***
  • Posts: 229
Re: Code review
« Reply #14 on: June 26, 2013, 08:51:42 AM »
Not right now, but maybe later. Do you remember the symptoms of the problem you observed?