WIP: Achievements#118
Conversation
|
I had a brief look at your approach: scanning all moves "live" as they happen. It has the advantage that the player could be alerted about any new achievements right away. Disadvantages include:
Another approach could be to record all player moves during the game. It shouldn't be too difficult as incremental player moves are already recorded for the undo feature. You could take the same data (+ end of turn markers) and record the whole game. The recording could be added to the GameExitedEvent and then checked for any unlocked achivements. The same data could also be used for other features, like offering to save the replay of a game for later re-watching or sharing. But that's certainly out of scope for this PR 😄 |
|
The undo was already giving me some headaches. I tried to solved it for the "buy n castles" case, but for other ideas, it would be much easier if we had a fixed linear history and just scanned through that to forward the progress of the achievements. Your have convincing arguments. I think that this suggestion is a great idea. Thus, I would scope this PR to just include achievements that work with only the current GameExitedEvent, which is a nice collection already IMHO. Everything else, I would remove again from the PR and we save that for a later modification. Or: We leave the existing castle-counting code of AchievementGameStateTracker in there, but not add anything else that uses this approach, as it allows for the (quite challenging) low-castle achievements and the "buy n castle" achievements -- but once we have that later modification with the recorded game we will refactor the code to use that and remove the AchievementGameStateTracker. The one disadvantage I see is that we have the AchievementGameStateTracker listening for three events and writing to preferences for each of them, which might have a performance impact. |
|
I noticed that I also subscribed to the RegenerateMapEvent in WinVeryHardGamesInARowAchievement. I guess that's also okay, it won't happen mid-game, not so often in total, and there's no "Undo". I need it to prevent people from skipping maps that look difficult to avoid breaking their streak. |
|
Known open issues:
|
|
I consider the localization nice to have for this, as it was added after this PR was already far along and you probably want to get it done soon. |
| magFilter: Nearest | ||
| characters: "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789()[]{}%&$§!?=+-.,:;_/\\*~#\"'↗–äöüÄÖÜßẞ\n" | ||
| } | ||
| MenuHeading: { |
There was a problem hiding this comment.
Is this font used somewhere? If not, it can be removed.
There was a problem hiding this comment.
It is used by menu_heading, defined in SkinConstants as FONT_MENU_HEADING, and used as the heading in the dialog that opens when you tap on a specific achievement. The important thing is that it is white. This is better readable in the dialog box with its turquois/cyan/green-blue (?) background.
There was a problem hiding this comment.
This file is generated and should not be edited directly. Instead, Skin Composer should be used. In addition to that, a new label style should be created and scaled in SkinFactory. Using the tool can be a bit tricky. Can I help you with that?
There was a problem hiding this comment.
If you mean "do everything for me", then you have my permission ;-). If you have some pointers, and I'll take it from there, I would also be glad. I guess "Skin Composer" is that tool (=a standalone program?).
| * Provides achievement classes, knows how to construct each individual achievement | ||
| */ | ||
| @Singleton | ||
| public class AchievementProvider { |
There was a problem hiding this comment.
I would probably call this AchievementFacory, as provider may be confused with the @provides concept of Dagger.
Speaking of which, there could be a provideAchievments method in a dagger module that uses this factory and makes the list of achievements injectable. That way, the AchievementService can be simplified as it only calls this method once anyway.
There was a problem hiding this comment.
That sounds interesting. I wasn't aware of this dagger concept.
| this.prefStore = achievementsPrefs; | ||
| } | ||
|
|
||
| public void LoadPersistedAchievements(Iterable<AbstractAchievement> achievements) { |
There was a problem hiding this comment.
Not sure I like that side effect on the achievements. Maybe the achievementProvider/achievement list can be injected into the repository directly instead of the service. Then this method could use that instead of the parameter and just return the populated list to the service.
There was a problem hiding this comment.
I refactored the code and now there is an AchievementsFactory with a Factory Method. I first wanted to use the dagger provides I had just learned about, but ultimately didn't use it.
My issue was that the AchievementsFactory "provides" a list of raw achievement classes. The AchievementRepository provides a list of achievement classes with the values loaded from the prefStore. I can only dagger-provide one of them (or use different collection classes to differentiate ... but that would be more of a hack). It felt unclean to me to provide the raw ones, because everywhere in the code except for the AchievementRepository itself, we'll want to have the ones with the loaded values. Thus, AchievementRepository uses the AchievementsFactory explicitly and calls the Factory Method. And AchievementService needs the reference to AchievementRepository anyway, so it feels redundant to have it injected both the AchievementsRepository AND the list of achievements. Hence, it also explicitly retrieves the achievements from the AchievementsRepository where it needs them and doesn't store a separate list of achievements. Consequently, the AchievementsSlide now gets its achievements for displaying also from the AchievementsRepository now and not the AchievementService.
… Generations and GameExit
… loaded from prefStore
…ievementsService and -Repository
|
I have addressed your concerns, @Sesu8642 . Please have a look, especially at the ones where I haven't yet resolved the conversation -- I didn't follow your suggestions by the letter in these cases and therefore I am not sure your concern is fully resolved. Everything is hard-coded English as of yet. The AI levels are shown as LEVEL1, LEVEL2, ... because the method converting them to readable display names wasn't there anymore -- I guess it needs a different approach now respecting the translations. This isn't even what it should be for English. |
|
Sorry for the delay. I'm focusing on release 1.5 right now, which I want to get out soon. I'll review your latest changes after that. |
|
It looks like there are some comments of mine from Mar 22 which you might have missed. Can you please have another look? |
| RICHARD_THE_LIONHEART("Richard the Lionheart", "Richard I. (1157-1199) was King of England. He fought many battles, especially in the Third Crusade, far away from his home."), | ||
| FREDERICK_THE_GREAT("Frederick the Great", "Frederick II. (1712-1786) was King of Prussia. While winning many battles, he is also known as a patron for education and the arts."), | ||
| LOUIS_XI("Louis XI", "Louis XI (1423-1483) was King of France and had the nickname 'The Universal Spider', as he carried out intrigues to play out his enemies against each other. He is known for his cunning and deviousness, but also for uniting France after the Hundred Years' War."), | ||
| HENRY_VIII("Henry VIII", "Henry VIII (1491-1547) was King of England. He is known for his six marriages, but also for building much military infrastructure."), // Reserved for BuyNCastlesAchievement |
There was a problem hiding this comment.
I noticed that the Henry ones are unused.
There was a problem hiding this comment.
Just saw your comments not. I guess they can stay until the castle achievement is implemented.
|
I started moving strings over to the translation files. Some auto-formatting happened. Please make sure to pull my changes to avoid conflicts. |
This PR is a Work in Progress for #101 Achievements.
I currently don't believe I require immediate feedback (although I am thankful for it), but here is where I track this task.
Status
So far, it implements the basic data and class structure for achievements. It also includes a UI page, which needs some attention. A couple of achievements are already added.
I rebased on February 19 after #116 was merged, therefore the initial commits all have that date.
Open Tasks
General/Architecture
UI
Achievements
We had lots of them defined and we may add others later. We need some "definition of done" for this PR and then we can add other achievements later on. There are many good ideas, but some of them will be hard to implement at least right now and it would take a lot of work to implement really all of them.
Achievements for which the current GameExitedEvent suffices
Achievements using castle counting- [x] WinWithOnlyNCastlesAchievement with n = 0, 1, 3- [x] BuyNCastlesAchievement with = 1, 20, 100Unit Tests