Code review

[Xela]

Do you remember the symptoms of the problem you observed?

There is no call to "remember", it becomes obvious the moment you change iterator name back to brothel and buy more than one brothel.

Try actually setting girls working in the brothels

[rudistoned]

I checked out revision c4ba128 (- Updated Traits File)
I bough two extra buildings
I set girls to work in these buildings
I clicked trough all events on next day

Nothing out of the ordinary happened.

[Xela]

I checked out revision c4ba128 (- Updated Traits File)
I bough two extra buildings
I set girls to work in these buildings
I clicked trough all events on next day

Nothing out of the ordinary happened.

After you buy several brothels:

Note that if iterator name in the girls assign screen is 'brothel':

1) When you set girls to work, tooltip displays faulty data.
2) Jobs available for girls in the brothel of your choice is not being updated either (Stripper job should be available to Mei in that brothel).

Note that problem disappears if you iterate with brtl (or anything else on the screen) with rest of the code being exactly the same, job is available, tooltip is functioning correctly.

[rudistoned]

Now I checked out the newest revision of master and changed "brtl" back to "brothel". Something should be wrong because the "brothel" variable of the label is used in the screen here:

Code: [Select]

    if brothel_selected:
        frame background Frame("content/gfx/bg/pergament.jpg",10,10):
            align (0,0.65)
            side "c r":
                viewport id "girlaction_vp":

<...>

                        if brothel.free_rooms() > 0 or chr.location == brothel:

                            for entry in brothel.actions:
                                    if entry == 'Stripper':

If "brothel" is used in the for loop in the screen code before my quoted code is executed, it would change which actual brothel the "brothel" variable in the quoted code refers to. I suppose that is the root of the problems you saw.

[Xela]

If "brothel" is used in the for loop in the screen code before my quoted code is executed, it would change which actual brothel the "brothel" variable in the quoted code refers to. I suppose that is the root of the problems you saw.

Prolly is the case.

[rudistoned]

This kind of bug could appear in any python script. However, it is IMHO more difficult to spot in renpy scripts due to the many levels of indentation. Pytfalls code makes it even more difficult because some lines of code are extremely long, so you have to scroll sideways to see all the code.

To avoid it, we should use more functions and classes, so less code is executed in the global namespace.

UPDATE: Does renpys screen language support functions and/or classes? They might call it custom widgets.

[Xela]

UPDATE: Does renpys screen language support functions and/or classes? They might call it custom widgets.

Supports pretty much anything. The entire screen can even be constructed in pure Python (if you wish to use the same paradigm as you've used in your Python OW port).

[rudistoned]

That's great :-)

[Xela]

That's great :-)

Yeap, having options is always a good thing. I personally prefer the looks of screen language itself, it's a lot less confusing once you get used to it. Running anything python in the screen language is also possible.

[rudistoned]

+            '''On Rudi's advice, I'll try to code some screen control into pure Python classes
+            This method enables/disables security bar, and sets max value depending on brothel upgrades
+            Index 0 = Show/No show of bar, index 1 = max value of bar
+            '''

 
 
What I meant to convey with my advise is that we should package GUI code up in classes and functions, so we don't overwrite global variables with variables that look "local", when in fact they are in the global namespace. Putting GUI code in game logic classes (anything else than GUI code) is quite the opposite of what I want 

With that being said, lets look at the complete method:

Code: [Select]

+        def gui_security_bar(self):
+            '''On Rudi's advice, I'll try to code some screen control into pure Python classes
+            This method enables/disables security bar, and sets max value depending on brothel upgrades
+            Index 0 = Show/No show of bar, index 1 = max value of bar
+            '''
+            result = []
+           
+            if len([girl for girl in self.get_girls() if girl.action == 'Guard']) > 0:
+                result.append(True)
+            else: result.append(False)
+           
+            if self.upgrades['mainhall']['2']['active']:
+                result.append(100)
+            elif self.upgrades['mainhall']['1']['active']:
+                result.append(50)
+            else: result.append(20)
+           
+            return result

Basically, this method answers two questions:
1) Is there a guard?
2) What's the maximum value for security at the bar?

So this method actually describes its instance and should therefore be part of the class. I would not consider that GUI code 

Here's another, IMHO clearer, way to answer the two questions:

Code: [Select]

        def is_guarded(self):
            '''Returns True if there is at least one guard here.
            '''
            guards = [girl for girl in self.get_girls() if girl.action == 'Guard']
            return bool(guards)

        def get_bar_security_max(self):
            '''Returns the maximum value for bar security (20 - 100).
            '''
            if self.upgrades['mainhall']['2']['active']:
                maxsec = 100
            elif self.upgrades['mainhall']['1']['active']:
                maxsec = 50
            else:
                maxsec = 20
            return maxsec

UPDATE: After reading Xelas post in the General discussion, I realize that "bar" probably refers to a GUI element. Well, my bad. It does not change anything important.

[Xela]

What I meant to convey with my advise is that we should package GUI code up in classes and functions, so we don't overwrite global variables with variables that look "local", when in fact they are in the global namespace. Putting GUI code in game logic classes (anything else than GUI code) is quite the opposite of what I want 

One of the things I like about RenPy is screen language and how it's setup. I figured you were in favor of cleaning screens up, not recoding them in pure Python.

[rudistoned]

I am in favour of cleaning the current screens up and I'm probably also in favour of using renpys screen language.


Is there any way to package screen language code into a separate namespace? Something like a subscreen or custom widget?



What I don't like is code that does something for screens in classes that contain game logic. Rule of thumb: if the class is in any of our current "classes - *.rpy" files, it should not contain GUI code. GUI code should IMHO be in the label that shows the screen, or in separate classes and functions.

[Xela]

I am in favour of cleaning the current screens up and I'm probably also in favour of using renpys screen language.

Is there any way to package screen language code into a separate namespace? Something like a subscreen or custom widget?

One or the other, you can't have both, would be to convenient.

What I don't like is code that does something for screens in classes that contain game logic. Rule of thumb: if the class is in any of our current "classes - *.rpy" files, it should not contain GUI code. GUI code should IMHO be in the label that shows the screen, or in separate classes and functions.

I honestly have no problem with it.

[rudistoned]

Code: [Select]

            # Should not be called (ever)
            else:
                txt += "Brothel is reasonably clean and there are no clients left to serve! "

This is a snippet from ServiceJob.set_task.

If the else condition should only be reached when something went wrong, why does it output ingame text instead of logging an error message?

[Xela]

If the else condition should only be reached when something went wrong, why does it output ingame text instead of logging an error message?

Because it still works, but feel free to remove it completely or turn it into an Error. Things like that are in many places in the code, especially in part that I've programmed close to when the project was started (Not having any previous coding experience of any serious kind).