General Discussion

[rudistoned]

I continued the discussion in the code review thread as it's rather involved already. Link: http://pinkpetal.org/index.php?topic=1891.msg21237#msg21237

[Xela]

Oki, did a bit of coding tonight:

- Wants to see strip/visit bar moved to create_customer() in Brothel
More customers would wanna see strippers/ go to bar if you have nice upgrades.

- Golden cages/Large Podium now have positive effects on strip job/tips/customer showup
Better upgrades = More tips = Better satisfaction = Happier cleints

- Some code refactoring
Removed a bunch of repetitive lines

- Security rating/Security Presence are added to Brothel
If there are active guards in brothel, player can tell them if they should make their presence known or not. Main Hall (max 50) and Reception (Max 100) are crucial here, same two upgrades also greatly decrease daily security rating degradation. Without these two upgrades max presence is set to 20. This way player has a measure of control on security rating. Tomorrow I am planning to remove any chance of attacks/brawls on maxed out security rating, however since those events can be reasonably profitable (costumer paying you off if he/she/they loose confrontation, it's up to player to decide.)

- Security Presence adjustment bar added to brothel management screen
Bar to adjust the presence, appears only when there are active security guards in the borthel.

- Changed theme (don't know why )
...

Plans for tomorrow/weekend:
- Finishing guard job
- Shop locations
- Wrapping up upgrades (Reception/Main hall/Statue Fame/Reputation and positive influence on Whore job effects are left)

- Rest of TODO: list follows, prolly girl control next.

[rudistoned]

I improved lots of code this weekend. You can find my changes in the faster branch on sourceforge: http://sourceforge.net/p/pytfall/pytfall/ci/faster/tree/

 My initial goal was to speed up switching from one event to the next in the next day screen. While reading through that code, I again stumbled over several things I think are problematic. Therefore, most commits in that branch restructure the next day logic and package it up into classes. In the last commit I somewhat achieved my inital goal.

Results: It turned out that most of the delay while switching to the next event results from loading the image. I subjectively "speed up" image loading by loading every image needed for the next day events before showing the next day screen. This results in an initial wait, but allows the player to switch between events very fast.  I also restructured the code into classes, making it more robust.

On my machine, the wait after clicking next day is quite long, easily more than 5s. Preloading all event images is not a good solution, so I don't think we should do that. I did include this change in the faster branch so you can check it out yourself. I might do some profiling later to see if something else also eats performance.

UPDATE:
profiling seems to indicate that renpy.image_size takes a lot of time (about 50 out 180s ?). I'll look into it a bit more.

@Xela
Please look at the changes and tell me to merge them into master 
Except for the image preloading, I think they improve the robustness und maintainability of our code. I also fixed a few bugs. I did some testing and so far have not found issues introduced by my changes. If you greenlight them, I'll do more testing before I merge.

[Xela]

I improved lots of code this weekend. You can find my changes in the faster branch on sourceforge: http://sourceforge.net/p/pytfall/pytfall/ci/faster/tree/

 My initial goal was to speed up switching from one event to the next in the next day screen. While reading through that code, I again stumbled over several things I think are problematic. Therefore, most commits in that branch restructure the next day logic and package it up into classes. In the last commit I somewhat achieved my inital goal.

Results: It turned out that most of the delay while switching to the next event results from loading the image. I subjectively "speed up" image loading by loading every image needed for the next day events before showing the next day screen. This results in an initial wait, but allows the player to switch between events very fast.  I also restructured the code into classes, making it more robust.

On my machine, the wait after clicking next day is quite long, easily more than 5s. Preloading all event images is not a good solution, so I don't think we should do that. I did include this change in the faster branch so you can check it out yourself. I might do some profiling later to see if something else also eats performance.

UPDATE:
profiling seems to indicate that renpy.image_size takes a lot of time (about 50 out 180s ?). I'll look into it a bit more.

@Xela
Please look at the changes and tell me to merge them into master 
Except for the image preloading, I think they improve the robustness und maintainability of our code. I also fixed a few bugs. I did some testing and so far have not found issues introduced by my changes. If you greenlight them, I'll do more testing before I merge.

I am focused on shops at the moment, I'll try to check our your code later. RenPy shouldn't load all images btw, We used to add path to the image and call it on the next index.

[rudistoned]

we used to add path to the image and call it on the next index.

I know. The problem with that approach is that it causes almost a second delay until it displays the next image on my laptop.

[Xela]

Ok:

- Guard events during jobs updated
- ItemShop(s) updated
- New Borthel Upgrades code added, all upgrades should have functionality now
- Brothel class updated a bit
- PyTFall World class created
- Shops inventory screen now shows shops/players gold
- Error on Next day fixed (bad else: in conflict resolver function)
- Auto define function for backgrounds fixed, same thing added for NPC (temporary, before NPC class is created)
- A number of new locations/screens created

Going to take a look at Rudi's code in 15 mins.

[Xela]

I know. The problem with that approach is that it causes almost a second delay until it displays the next image on my laptop.

Next Day takes unacceptably long time, especially with a lot of events (under faster branch). That code must never make it to master branch. We'll find another way to speed things up.

Edit: It takes 49 seconds for Next Day to load under your code with 106 acts. It takes less then 3 seconds to load 1000 acts under old code. This is not a solution... this is a game suicide

And looking back @ your post:

Preloading all event images is not a good solution, so I don't think we should do that.

you seem to have come to the same conclusion...

Edit 2: Only this bit is not useful:

Code: [Select]

                # preload all images (renpy handles the caching)
                for evt in self.allevents:
                    evt.load_image()
Rest seems alright if it doesn't create any new Errors...

Edit 3: Please halt on merging branches. I've ran a series of tests and faster branch (without image preloading) for some reason generates only 80 - 100 acts while the master branch generates 800 - 900 acts under the same circumstances. I've been coding for the better part of the day as well (as you can prolly see from the update) so I don't have any strength left to track down why.

[Xela]

Edit: It takes 49 seconds for Next Day to load under your code with 106 acts. It takes less than 4 to load 906 events with old code. This is not a solution... this is a game suicide

I've implemented temporary fix for your Next Day index slowdown problem (should be nearly instantaneous now), it can be farther improved upon but we should code towards 1.0 and then improve existing code (something I've requested many times already).

[rudistoned]

It takes 49 seconds for Next Day to load under your code with 106 acts. It takes less then 3 seconds to load 1000 acts under old code. This is not a solution... this is a game suicide

Thats just not true.  faster branch including the last commit loads every image needed for next day events before it shows the next day screen. master loads the images while you are clicking through them, so master should take about 460s spent on image loading while you click through the events.
So, the faster branch implementation is especially bad with many events because it might overflow the image cache. The idea of faster branch is to show you guys how fast switching between events can be when the image is already loaded. This behaviour is solely caused by the last commit and it was never my intention to merge it into master. I might have stated that more clearly...

The only way the implementation in master (prior to your fix) can be faster is if the player does not look at all events.

I've implemented temporary fix for your Next Day index slowdown problem (should be nearly instantaneous now)

Your fix improves things as the game loads less images now, but it's still nowwhere near instanteneous on slower machines like my laptop. Why do you call that fix temporary? If the game should always show a profile picture if no better picture is available, this is a change for the better and should be permanent.
UPDATE: I suppose you mean that the code can and should be simplified further based on this new information (new to me anyway).

It's not a gamebreaker, but IMHO it is a serious problem. The speed of harddisks improves very slowly and SSDs are still too expensive to be everywhere, so faster hardware is not going to solve this problem. We'll need to tackle it sooner or later. As a player, I would not read the events often if it takes so long to load them.

Please halt on merging branches. I've ran a series of tests and faster branch (without image preloading) for some reason generates only 80 - 100 acts while the master branch generates 800 - 900 acts under the same circumstances. I've been coding for the better part of the day as well (as you can prolly see from the update) so I don't have any strength left to track down why.

I'll look into this.

# select_image method should be rewriten to reflect realities of the game and require less calculation time

Do you have any support for your claim that the select_image method requires too much calculation time? And how much is too much?
You guys have talked about that way of choosing an image being slow repeatedly, but nobody ever provided any evidence.

My profiling data says select_image requires a ratio of 0.0005, or 0.05 %, of runtime as calculation time. How could that be too much? We spend almost as much time just so we can have our own wrapper for the notify function. We also spend 10 times as much time in the dice function as in the select_image method. However, improving any of those mentioned to reduce total runtime is pointless.

[Xela]

Thats just not true.  faster branch including the last commit loads every image needed for next day events before it shows the next day screen. master loads the images while you are clicking through them, so master should take about 460s spent on image loading while you click through the events.
So, the faster branch implementation is especially bad with many events because it might overflow the image cache. The idea of faster branch is to show you guys how fast switching between events can be when the image is already loaded. This behaviour is solely caused by the last commit and it was never my intention to merge it into master. I might have stated that more clearly...

On my 5 year old laptop, each act on next day took little under a second to load (noticeable delay before the fix). On desktop, I wouldn't even consider it a problem.

But it's true... non the less. Next day loading several minutes (we should get 1k events at latter points of development), even if index loading is extremely fast afterwards is a deal breaker and kills the game. If next day is loaded in 3 secs, player can use filters (once we have now and we'll install more in the future), he/she will be able to actually read and view pics/stat increases and close the next day, going through with the game. There is no point in claiming that both take long time to the same effect, time it takes to load and view all events might be comparable, but the game play experience is perfectly acceptable in one case and absolutely sh#tty in the other.

Your code actually cashes the images after you view them once, so you could have simply told us to go through a couple of images and revert to previous indexes to see the difference in speed

The only way the implementation in master (prior to your fix) can be faster is if the player does not look at all events.

Like I've said, game experience, we're not coding some theoretical modeling software, last I've checked, we were working on a game.

Your fix improves things as the game loads less images now, but it's still nowwhere near instanteneous on slower machines like my laptop. Why do you call that fix temporary? If the game should always show a profile picture if no better picture is available, this is a change for the better and should be permanent.

My bad, my laptop is 5 years old, if not more, I've tested it there as well and did not notice anything I might have called a delay (not even mentioning how it runs on desktop). It does have dedicated Nvidia GPU (old one but maybe it makes it run faster or something).

The approach could be permanent but it feels like select_image should handle all image selecting logic. I haven't traced the entire logical flow (was to tired). Maybe things are fine the way they're now.

It's not a gamebreaker, but IMHO it is a serious problem. The speed of harddisks improves very slowly and SSDs are still too expensive to be everywhere, so faster hardware is not going to solve this problem. We'll need to tackle it sooner or later. As a player, I would not read the events often if it takes so long to load them.

I ran this on SSD, next day index works lighting fast, both on it and on normal harddrive. But I didn't know it was still slow so another look a the entire chain to see if it can be farther improved upon.

Do you have any support for your claim that the select_image method requires too much calculation time? And how much is too much?
You guys have talked about that way of choosing an image being slow repeatedly, but nobody ever provided any evidence.

My profiling data says select_image requires a ratio of 0.0005, or 0.05 %, of runtime as calculation time. How could that be too much? We spend almost as much time just so we can have our own wrapper for the notify function. We also spend 10 times as much time in the dice function as in the select_image method. However, improving any of those mentioned to reduce total runtime is pointless.

You guys have talked about that way of choosing an image being slow repeatedly, but nobody ever provided any evidence.

I've NEVER said that (at least while I was sober). I though that we needlessly call it twice... and select_image should simply always return 'profile' if no other tags are found. (it should take less calc time, but I've never said it was slow to begin with. Prolly poor choice of words in my comments at 2 am yesterday )

But if you did actual profiling and those were the results, it's all good. I've never been in favor of improving working code instead of advancing the game.


=================================================
Edit: You're right, it is still noticeable on laptop (just tried it again) Maybe I was slower yesterday so it seemed fast 

I'll take another look to see if things can be farther improved, but I would still call it acceptable on Laptop and perfect on Desktop.

[rudistoned]

Next day loading several minutes (we should get 1k events at latter points of development), even if index loading is extremely fast afterwards is a deal breaker and kills the game. If next day is loaded in 3 secs, player can use filters (once we have now and we'll install more in the future), he/she will be able to actually read and view pics/stat increases and close the next day, going through with the game. There is no point in claiming that both take long time to the same effect, time it takes to load and view all events might be comparable, but the game play experience is perfectly acceptable in one case and absolutely sh#tty in the other.

I agree 100%. What is not true is your claim that the faster branch takes 49 seconds to load 106 acts while master branch takes 3 seconds to load 1000 acts 

Your code actually cashes the images after you view them once, so you could have simply told us to go through a couple of images and revert to previous indexes to see the difference in speed

Yes, I should have done that, but I did not think of this easy solution at that time.

Like I've said, game experience, we're not coding some theoretical modeling software, last I've checked, we were working on a game.

Yes, we are. How does that make your claim that master takes less time to load acts any more true? 
Again, I never said preloading all images for all acts before showing the next day screen is better for game experience.

My bad, my laptop is 5 years old, if not more, I've tested it there as well and did not notice anything I might have called a delay (not even mentioning how it runs on desktop).

Maybe you are more patient than I am. A reaction time of almost a second is a serious delay in my book and quite annoying while playing a game.

It does have dedicated Nvidia GPU (old one but maybe it makes it run faster or something).

GPUs should have no influence at all on the speed of our game. The IMHO singlemost important cause for the delay after clicking "Next Event" is loading the image from the harddisk. What's worse is that our image resize function does not use renpys image cache.

Possible ways to speed things up:
1) Find a way to resize images in renpy while using the image cache
2) instantiate a renpy Image when an event is created, so renpy has more time to preload and cache the picture

The approach could be permanent but it feels like select_image should handle all image selecting logic. I haven't traced the entire logical flow (was to tired). Maybe things are fine the way they're now.

I've NEVER said that (at least while I was sober).

You did not say it recently, but as far as I remember it took quite some convincing that it's not terribly slow when I first presented that idea to you. Back then we were still talking about Pytherworld. Anyway, maybe I just don't remember that right, it does not matter.
mijh definitely said its slow and is strongly opposed to this image selection strategy. Of course, he is right that pytfalls original approach is faster, but it does not matter if we spend 0.01% or 1% of our calculation time on image selection. Both numbers are small enough that they won't affect the game performace-wise.
 

But if you did actual profiling and those were the results, it's all good.

I did, using cProfile. I'm sure my results are accurate enough to say that select_image is no problem for game performance at this point.

I'll take another look to see if things can be farther improved, but I would still call it acceptable on Laptop and perfect on Desktop.

You can leave that to me if you want to. I spent some time reading renpys sparse documentation and nice source code in order to understand renpys image caching. As far as I can tell, renpy preloads images sometime between instantiating the renpy Image and displaying it on the screen. Instantiating a renpy image as soon as possible should improve our situation.
I also saw that the image cache seems to know the size of the image it holds, so if we can figure out a way to get our image from the cache, we should be able to resize it without using renpy.image_size.

[Xela]

I agree 100%. What is not true is your claim that the faster branch takes 49 seconds to load 106 acts while master branch takes 3 seconds to load 1000 acts 

I am not making that up, I've timed it I am talking about the time it takes Next Day to complete all calculations, no including paging through indexes obviously.

You did not say it recently, but as far as I remember it took quite some convincing that it's not terribly slow when I first presented that idea to you. Back then we were still talking about Pytherworld. Anyway, maybe I just don't remember that right, it does not matter.

LoL

It's wasn't me, you argued with compuscribe about the whole tagging concept on old OW forum. I actually thought the idea was very interesting.

[DarkTl]

Xela, what are characteristics of your laptop?

[Xela]

Xela, what are characteristics of your laptop?

2.4 Ghz Core Duo with 4GB of DDR2.

[rudistoned]

I am not making that up, I've timed it I am talking about the time it takes Next Day to complete all calculations, no including paging through indexes obviously.

I'm not saying you made it up. I'm saying the faster branch takes 49 seconds to complete the next day calculations and load all images while the master branch takes 3 seconds to  complete the next day calculations. It's an unfair comparison because the faster branch is doing more work. That's why claiming the faster branch is slower for a given number of acts is wrong 

It's wasn't me, you argued with compuscribe about the whole tagging concept on old OW forum. I actually thought the idea was very interesting.

I did argue with compuscribe, although our argument was more about XML vs. XMP tags. Well, sorry, I might have confused that.

2.4 Ghz Core Duo with 4GB of DDR2.

Pretty similar to mine. I think that's a good benchmark. If the game runs okay on a machine like that, we should be fine. Not many people will try to play the game on slower hardware, unless we want to support netbooks and tablets too (which we should not IMHO).