Savegamesimulator #175

oezgueremir · 2020-07-25T09:38:01Z

Integrate Savegame in TestUnit

Savegame functionality tested:

save game
query available savegames
load savegame
delete savegame

Also done some (mostly automatic) clean ups...

Added functionality: - Save and load game - Enemy definition overview - Activate game speed via checkbox - Enable game speed from 2 to 128 in steps of [2, 4, 8, 16, 32, 64, 128] ("power of two") Feel free for review and comments

Savegame functionality tested: - save game - query available savegames - load savegame - delete savegame Also done some (mostly automatic) clean ups...

mjaun · 2020-07-26T17:25:51Z

Hi. Thanks again for your contribution. Before diving into the code I would like to make some general comments.

First, for your next contribution it would be nice to have the individual improvements also as individual pull requests, possibly containing multiple smaller commits. This makes it easier to discuss and approve them.

Save Game: In general I like how the load/save game menus are integrated. In the load game menu I suggest to show only the date and the score but in turn make the screenshots bigger such that the look and feel is close to the change map menu. Also would it be hard to only capture the game view without the menus?

Game Speed: Since this is the second PR wanting to run the game faster than 4x, I agree to change this. 128x is maybe a little bit overkill though =) I also think that it is necessary that you can always go back to 1x with a single click. But the checkbox as it is isn't very self explanatory in my opinion. What about a toggle button with the label "Fast" with the same functionality?

Enemy FAQ: Nice idea, especially to show the strengths and weaknesses of an enemy. I suggest not to show speed, reward and health because some enemies do not move at a constant speed and reward/health increases over time. Also there seems to be an issue with the layout on very small screens (tried with a Nexus One simulation).

What do you think about my suggestions?

oezgueremir · 2020-07-26T23:19:36Z

Hi! First of all, thanks for the review.

About the package extent, sorry about that.
I didn't want to make much fuss many PRs with too little packages.
In hindsight, it would have been better, yeah. Again, sorry.

If you want, close this PR. Then I would split my commits, so we can lead a more focused discussion?

Savegame
"capture the game view without the menus", you mean without the HeaderFragment, right? Because the regular game menus (MenuActivity, TowerInfoFragment, ...) shall never be visible in the screenshot. Except in the test unit project, for some reason...
Well, that is possible, yes. I even suggest to clip the screenshot, so only the core game map is visible without any padding around. I will implement that, as soon as I sent this comment.

Even though I get your concerns about the consistency and visibility, I'd like to show more than only the timestamp and the points. At least wave number and remaining lives should be given to the user. As a compromise, how would you like a "split view"? Instead of a grid view with two columns of savegames in each line, I could build a single column for each line with some textual information on the left and a little bigger screenshot view on the right.
As there is yet another idea on my to do list about grouping the savegame by run ("new game", the split view would be even better.
Another possibility would be, to maximize the screenshot on the first click and load the game on the second click on the enlarged view.

Game Speed
Only the sky is the limit, so... ;-)
This was also a request of a friend of mine. If I remember correctly, he wanted at least 32x speed.
Since I got complaints about sizing in the HeaderFragment (tower building became a popup menu), I gave up the label text of the check box. But your idea could be merged with my current solution.
I would build the button back, as it was (normal <-> fast). But when active (or on "longclick"?) there would pop up a slider or something, where you could define what "fast" means.

Enemy FAQ
Well, if you think so, I don't have a problem with hiding some non-static information. But I think anybody would understand, that the shown enemy data is meant in relation to each other enemy type, not as a absolut value for each wave. I will take a look on the visualization on smaller devices.
To be true, I thought about integrating even more informations. Based on the wiki, including other aspects than just emenies.

Btw, my local branch has even more improvements:

Always show enhancement level, upgrade level and tower strategy (like LevelIndicator, but even more info)
Automatically call next wave (since I am/we are used to that behaviour by other TD games)
mass update/upgrade towers (due to a request)
relay strategy of selected tower to others (due to a request)
and more, even more on my growing to do list.

Well, I think, at least some of them would break the main philosophy of your game, so these will never end up in an PR...

mjaun · 2020-07-27T18:37:50Z

Splitting the PR I think we can sort it out for this PR. Just keep it in mind because it makes it easier for the reviewer =)

Save Game Split view could get tricky for small screens. I think just enlarging the screenshot a bit would do it for a first version.

Game Speed What I meant was using a toggle button (like a regular button but which remains "pressed") instead of a checkbox. Longclick is hard to figure out if there is no indication. And of course the build menu should be avoided if possible.

Enemy FAQ Ok, let's keep the information. What I forgot to mention is that I would probably rename the menu to something else like "Enemy Stats", because a typical FAQ contains questions and answers.

As for the other improvements I am open for everything but if it is something which the average user doesn't need then I would like to have it configurable in the settings such that the game remains easy to use in its default configuration.

mjaun · 2020-07-27T18:56:25Z

app/src/main/java/ch/logixisland/anuto/AnutoApplication.java

        sInstance = this;
        mGameFactory = new GameFactory(getApplicationContext());
    }

+    public static Context getContext() {


No usages. Probably a left over.

This is indeed unnecessary in this branch, currently only used in my local branch.

mjaun · 2020-07-27T18:59:11Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

@@ -86,7 +102,7 @@ public void execute() {
    }

    public void saveGame() {
-        if (mGameEngine.isThreadChangeNeeded()) {
+        if (mGameEngine.isThreadRunnging() && mGameEngine.isThreadChangeNeeded()) {


Why is this necessary, in which case is the game thread not running?

There was one case, where this additional clause improved something...
I can't remember exactly what it was... I will investigate that.

btw. "Runnging"? Sorry for the typo...

I remembered the reason, as I described it for "makeNewSavegame".
Some time ago this if-clause was in the "private void saveGame(..", so it was necessary, since it is called by both, "public void saveGame()" and "public File makeNewSavegame()". Since I have moved the if-clause one level up, the "isThreadRunnging" should be obsolete here, as long as this is not called via another activity.

mjaun · 2020-07-27T19:03:17Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

+        saveGame("", false);
+    }
+
+    private void saveGame(final String rootdir, final boolean userSavegame) {


Not very clear because rootdir is ignored if userSavegame is false. Would simply pass the file name as argument.

Yet another thing, needed for my local branch. Can be changed, of course.

mjaun · 2020-07-27T19:09:34Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

+                }
+            });
+            return;
+        }*/


Why is this disabled, for capturing the screenshot? At least the saveGame() method must run on the game thread otherwise there will probably be issues with concurrently accessing the game objects.

makeNewSavegame shall only be called via "MenuActivity".
Therefore the thread isn't running at all.
Therefore this if-clause is always FALSE and so it is redundant.

And now I remember the reason for "isThreadRunning" at all.
Assumeed this code is active and "isThreadRunning" is not checked.
The thread is already paused at startup of the "MenuActivity". When you call "makeNewSavegame" from the "MenuActivity", this command will be added to the message queue, which will be worked down when the game continues.

So, you click on "save game" and it is not executed at once, but after you close the "MenuActivity".

I understand. Will have another look to see if there is a more explicit solution, but if not I think it is fine.

mjaun · 2020-07-27T19:18:01Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

+                one.delete();
+            }
+            rootdir.delete();
+        }


I would simply delete the three files and then the directory and log a message if something cannot be deleted. Seems easier to me than to make sure that the directory contains exactly the three files beforehand (which could also be simplified by comparing two HashSets).

I got a little "over anxious" when it came to delete something at development time.
So I just left the checks there, since it has already been impemented.

Btw. I am not a native Android/Java developer, so I am sure there are and always be potential for improvement :-D

Sure, I think everybody's code has potential for improvement =)

mjaun · 2020-07-27T19:59:49Z

app/src/main/java/ch/logixisland/anuto/entity/enemy/EnemyProperties.java

+
+            this.setEnemyType(EnemyType.valueOf(entityName));
+
+            switch (mResult.getEnemyType()) {


I guess you moved the information here to also have access to it in the Enemy FAQ. The downside of these kind of switch cases is that if someone wants to add a new type of enemy, he has to find all the relevant switch cases in the code base and adapt them. I think ideally, in order to add a new type of enemy, you should just have to write your new enemy class and register it in one place (GameFactory) which was the case before.

To access the information in the Enemy FAQ we can also use the EntityRegistry and add a way there to grab all the EnemyProperties.

Yeah, I tried to use the properties directly from the Enemy Entity, but only the private builder had all the necessary informations. And it ached to build an instance of an Enemy, just to collect its properties.

The EntitiyRegistry would be even better. I just didn't fancy that much trouble :-D

No issues, I can also have a look at it later.

mjaun · 2020-07-27T20:02:33Z

app/src/main/java/ch/logixisland/anuto/entity/enemy/WeaponType.java

@@ -4,5 +4,6 @@
    None,
    Bullet,
    Laser,
-    Explosive
+    Explosive,
+    Glue


Now that Glue is officially a WeaponType it would be nice if the GlueEffect class would check for the strong against Glue instead of ignoring Flyer instances specifically.

Absolutely!
I thought about that, as well.
But I couldn't check, if there are any other ricochet if the current behavour is changed.

mjaun · 2020-07-27T20:05:55Z

app/src/main/java/ch/logixisland/anuto/util/StringUtils.java

        }

-        DecimalFormat fmt = (value < 1e2f && (!integer || big)) ? fmt1 : fmt0;
+        DecimalFormat fmt = (usePointedFmt) ? fmt1 : fmt0;


What is the reasoning for this change?

I worked on an option, where the score is always shown and had issues in the whole process.
This is some leftover code change.
It can be reverted, if you want.

mjaun · 2020-07-27T20:08:10Z

app/src/main/java/ch/logixisland/anuto/view/faq/EnemiesAdapter.java

+    private Bitmap extractSingleBmp(EnemyType enemyType) {
+        int attrId, spriteCount, spriteId;
+
+        switch (enemyType) {


Same argumentation as for the EnemyProperties. Would be better to get the bitmaps directly from the Enemy classes using the EntityRegistry.

Sure, but won't that be to much of an hassle?

mjaun · 2020-07-27T20:08:41Z

app/src/main/java/ch/logixisland/anuto/view/faq/EnemiesAdapter.java

+        ViewHolder viewHolder = new ViewHolder(enemyItemView);
+        String dmp;
+
+        switch (enemyProperties.getEnemyType()) {


Same argumentation as for the EnemyProperties. Would be better to get the bitmaps directly from the Enemy classes using the EntityRegistry.

mjaun · 2020-07-27T20:18:12Z

I also went through the code today and made some comments. I don't expect you to address everything I mentioned. Just tell me if you are finished with the cleanups from your side then I will merge the changes and will have a look at the remaining things. Any feedback for my comments is of course also appreciated.

oezgueremir

I hope, I haven't missed any comment to answer to.

oezgueremir · 2020-07-27T20:39:53Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

+                }
+            });
+            return;
+        }*/


makeNewSavegame shall only be called via "MenuActivity".
Therefore the thread isn't running at all.
Therefore this if-clause is always FALSE and so it is redundant.

And now I remember the reason for "isThreadRunning" at all.
Assumeed this code is active and "isThreadRunning" is not checked.
The thread is already paused at startup of the "MenuActivity". When you call "makeNewSavegame" from the "MenuActivity", this command will be added to the message queue, which will be worked down when the game continues.

So, you click on "save game" and it is not executed at once, but after you close the "MenuActivity".

oezgueremir · 2020-07-27T20:46:43Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

@@ -86,7 +102,7 @@ public void execute() {
    }

    public void saveGame() {
-        if (mGameEngine.isThreadChangeNeeded()) {
+        if (mGameEngine.isThreadRunnging() && mGameEngine.isThreadChangeNeeded()) {


I remembered the reason, as I described it for "makeNewSavegame".
Some time ago this if-clause was in the "private void saveGame(..", so it was necessary, since it is called by both, "public void saveGame()" and "public File makeNewSavegame()". Since I have moved the if-clause one level up, the "isThreadRunnging" should be obsolete here, as long as this is not called via another activity.

oezgueremir · 2020-07-27T21:06:38Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

+
+        GameFactory daFac = AnutoApplication.getInstance().getGameFactory();
+        WaveManager mWaveManager = daFac.getWaveManager();
+        ScoreBoard mScoreBoard = daFac.getScoreBoard();


I just went the way of the least resistance ;-)
I exactly tried your suggested way and, as you probably saw, already done that to other classes.
But sometimes I got in trouble because of some cyclic dependencies.

oezgueremir · 2020-07-27T21:13:35Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

+                one.delete();
+            }
+            rootdir.delete();
+        }


I got a little "over anxious" when it came to delete something at development time.
So I just left the checks there, since it has already been impemented.

Btw. I am not a native Android/Java developer, so I am sure there are and always be potential for improvement :-D

oezgueremir · 2020-07-27T21:16:42Z

app/src/main/java/ch/logixisland/anuto/business/game/GameLoader.java

+        }
+    }
+
+    public KeyValueStore readSaveGame(final String fileName, final boolean userSavegame) {


Yeah, sure.
Shall only be needed in local branch.

oezgueremir · 2020-07-27T21:39:49Z

app/src/main/java/ch/logixisland/anuto/business/wave/WaveManager.java

@@ -93,8 +99,6 @@ public void execute() {
        updateRemainingEnemiesCount();

        setWaveNumber(mWaveNumber + 1);
-        setNextWaveReady(false);
-        nextWaveReadyDelayed(NEXT_WAVE_MIN_DELAY);


And again...local branch...sorry...
If the ready flag is not set this early, my "auto wave" routine starts about 12 waves at once.

But for your branch, this can be built back.

oezgueremir · 2020-07-27T21:47:56Z

app/src/main/java/ch/logixisland/anuto/entity/enemy/EnemyProperties.java

+
+            this.setEnemyType(EnemyType.valueOf(entityName));
+
+            switch (mResult.getEnemyType()) {


Yeah, I tried to use the properties directly from the Enemy Entity, but only the private builder had all the necessary informations. And it ached to build an instance of an Enemy, just to collect its properties.

The EntitiyRegistry would be even better. I just didn't fancy that much trouble :-D

oezgueremir · 2020-07-27T21:51:07Z

app/src/main/java/ch/logixisland/anuto/entity/enemy/WeaponType.java

@@ -4,5 +4,6 @@
    None,
    Bullet,
    Laser,
-    Explosive
+    Explosive,
+    Glue


Absolutely!
I thought about that, as well.
But I couldn't check, if there are any other ricochet if the current behavour is changed.

oezgueremir · 2020-07-27T21:58:46Z

app/src/main/java/ch/logixisland/anuto/util/StringUtils.java

        }

-        DecimalFormat fmt = (value < 1e2f && (!integer || big)) ? fmt1 : fmt0;
+        DecimalFormat fmt = (usePointedFmt) ? fmt1 : fmt0;


I worked on an option, where the score is always shown and had issues in the whole process.
This is some leftover code change.
It can be reverted, if you want.

oezgueremir · 2020-07-27T22:01:07Z

app/src/main/java/ch/logixisland/anuto/view/faq/EnemiesAdapter.java

+    private Bitmap extractSingleBmp(EnemyType enemyType) {
+        int attrId, spriteCount, spriteId;
+
+        switch (enemyType) {


Sure, but won't that be to much of an hassle?

oezgueremir · 2020-07-27T22:31:41Z

Save Game
Well, I will write "split view/enlarged screenshot" in my to do list and propose it as another PR in the future, so we can discuss it further ;-)
So, to wrap it up, in this Version there will be following?

croped screenshot
timestamp
score
wave
lives

Game Speed
Yeah, I got that with toggleable button. But how do you suggest adjusting the multiplicator value? Just like now, with another button to cycle through the multipliers? So, you just want to replace the Checkbox by a toggle button?

Enemy FAQ
Got it, will be changed.

Shall I commit the adjustments in my SavegameSimulator branch, so you can take a look, or how should we continue?

Other improvements
Wow. Didn't expected this, cool, thanks!
As requested by my "users", most of these are already de-/activatable in the settings, default=off

mjaun · 2020-07-28T20:44:33Z

Save Game
Sounds good.

Game Speed
Yes, replacing the checkbox by a toggle button is what I would try.

Adjustments
If you push them to your SavegameSimulator branch they should be automatically added to this pull request which would be the preferred way to go for me.

Other Improvements
Sure, I'm happy if someone is willing to contribute.

Edit: Will go through your code comments later.

show only - timestamp - score - wave - lives and crop screenshot to maximize map visibility

- renamed 'faq' to 'stats' - resized to run on smaller displays aswell - implemented getStrongAgainst <-> WeaponType.Glue - added some translations - "Reformat Code"

oezgueremir · 2020-07-30T23:41:58Z

I tried to replace the game speed CheckBox with an ToggleButton, but when you change the height, so it is consistent to the other buttons in the HeaderFragment, the indicationbar overwrites the buttontext.
We could resize the HeaderFragment but I don't know the impact on the GameView on smaller devices.

Talking about smaller devices, I resized the EnemyStatsActivity (formerly known as FAQActivity). But since I cannot start an emulator for the mentioned Nexus One (non-capable CPU) I have to confide in the designer view. So, if you could kindly recheck that...

mjaun · 2020-08-01T13:52:16Z

app/src/main/java/ch/logixisland/anuto/engine/render/Renderer.java

+
+        //perhaps build another draw routine, so the game map is better visible
+        //e.g. an independent theme just for savegame screenshot
+        draw(canvas);


I actually like this approach very much!

Thanks. And I am serious in my comment.
I already tried some stuff, but never really got satisfied.

A forth, neutral theme would have the positiv effect, that the load game menu would look homogeneous, no matter with which theme the game runs in the meantime.

Of course, beside the aim for a better screenshot visualization of the game map...

mjaun · 2020-08-01T13:53:55Z

app/src/main/java/ch/logixisland/anuto/entity/enemy/Enemy.java

+        if (origin instanceof Tower) {
+            Tower originTower = (Tower) origin;
+
+            if (!mEnemyProperties.getStrongAgainst().contains(originTower.getWeaponType())) {


Why not doing this check in GlueEffect?

You are right, since modifySpeed is only used by GlueEffect, I thought about that approach, too.

But I voted for the current solution, since the getWeakAgainst/getStrongAgainst check is here in Enemy-damage, as well.

And, well, who knows what cool new Tower the future will bring us :-)

mjaun · 2020-08-01T14:02:36Z

Hi, thanks for your latest improvements!

Regarding the CheckBox / ToggleButton: I will also have a look at it. What would be your recommendation? Keeping the CheckBox?

I will check again for the EnemyStatsActivity.

If it is ok for you, I would merge the current state and start working on it. Meaning you will have to create new pull requests if you want to add more changes and sync with my repository from time to time. Can we proceed like that or would you like to add something to this pull request?

oezgueremir · 2020-08-01T20:54:26Z

CheckBox / ToggleButton
Unfortunately, I am out of easy ideas, too. Perhaps influencing the multipicator button, so it is drawn differently when the CheckBox is in-/active? But that could bring another confusion factor...

What about a Split DropDown Button? The main area on the left could swich and print "on/off". And the multiplicator could be selected in the DropDown menu

Or a Segmented Button? Left side = "on/off", right side = multiplicator

As for merging, that would work with me, I have nothing usable to add to this pull request.

And I have no problems submitting another PR. Actually, I look forward to it.

oezgueremir added 3 commits July 24, 2020 00:14

Savegame, Enemy FAQ & Gamespeed checkbox

e3de01b

Added functionality: - Save and load game - Enemy definition overview - Activate game speed via checkbox - Enable game speed from 2 to 128 in steps of [2, 4, 8, 16, 32, 64, 128] ("power of two") Feel free for review and comments

Merge remote-tracking branch 'refs/remotes/origin/Savegame'

7f9d168

Integrate Savegame in TestUnit

c5c68f2

Savegame functionality tested: - save game - query available savegames - load savegame - delete savegame Also done some (mostly automatic) clean ups...

mjaun mentioned this pull request Jul 26, 2020

Savegame, Enemy FAQ & Gamespeed checkbox #174

Closed

mjaun requested changes Jul 27, 2020

View reviewed changes

oezgueremir commented Jul 27, 2020

View reviewed changes

oezgueremir added 2 commits July 30, 2020 22:59

save game refinement

943551b

show only - timestamp - score - wave - lives and crop screenshot to maximize map visibility

FAQ - WeaponType.Glue - Strings

81e170c

- renamed 'faq' to 'stats' - resized to run on smaller displays aswell - implemented getStrongAgainst <-> WeaponType.Glue - added some translations - "Reformat Code"

mjaun reviewed Aug 1, 2020

View reviewed changes

mjaun approved these changes Aug 1, 2020

View reviewed changes

mjaun merged commit b5efffd into mjaun:master Aug 4, 2020

mjaun mentioned this pull request Aug 10, 2020

Game Speed 3 levels, 1-4-8 #162

Closed


		this.setEnemyType(EnemyType.valueOf(entityName));

		switch (mResult.getEnemyType()) {

Savegamesimulator #175

Savegamesimulator #175

Conversation

oezgueremir commented Jul 25, 2020

mjaun commented Jul 26, 2020

oezgueremir commented Jul 26, 2020

mjaun commented Jul 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjaun commented Jul 27, 2020

oezgueremir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oezgueremir commented Jul 27, 2020

mjaun commented Jul 28, 2020 • edited Loading

oezgueremir commented Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjaun commented Aug 1, 2020

oezgueremir commented Aug 1, 2020

mjaun commented Jul 28, 2020 •

edited

Loading

oezgueremir commented Jul 30, 2020 •

edited

Loading