Closed oezgueremir closed 4 years ago
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?
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:
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...
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.
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.
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?
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
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.
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...
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?
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.
Integrate Savegame in TestUnit
Savegame functionality tested:
Also done some (mostly automatic) clean ups...