Feature: Implements 'themes' #52

BayMinimum · 2018-10-12T04:07:01Z

Added Themes class to handle theming and applied to game (#51)

Added Themes class to handle theming and applied to game

cawvyoct · 2018-10-15T20:53:53Z

Thank you for your contribution! 🎉
May I try to code review your PR?
The intention of the code review will be to help foster more PRs from you.
And if so, do you have any concerns from doing the code review which may unintentionally discourage you?

BayMinimum · 2018-10-15T23:52:12Z

Of course yes! Please do the review 🙏

plibither8 · 2018-10-16T04:33:07Z

@cawvyoct Thank you! I've been quite busy this week, I would love your help 😄

cawvyoct · 2018-10-17T19:57:35Z

So, I don't know what level of C++ you know so I will try and keep things generic.

Again, congratulations on reaching this far with your C++ development! It's a good start to greater things 🥇 !

By the way, questions are highly encouraged! ❔ 😃 👍 !
It helps to understand if I didn't explain things in a "digestible" way for you.
I may not suggest exact C++ code to fix problems but I will hint of some C++ techniques which may help you on your way to solve some issues. 🎉 🗺️ 🔍

cawvyoct · 2018-10-17T20:00:06Z

Overall tl:dr: try to imagine if contributors added 400 themes to this game, would the code added in the PR help to abstract all the themes so they can be be easily added without "much" problems.

cawvyoct · 2018-10-17T20:07:14Z

Overall tl:dr-2: It is always a challenge to define what is an "interface" (API) and what is "data".
It is also a challenge to make things generic (in this case, in C++) for different "applications".

For example, one "application" means "generic for programmers" to read (and understand the source code / flow).
Another "application" which means "generic for contributors" to edit and add their own content without needing to understand all the source code in totality.

cawvyoct · 2018-10-17T20:22:11Z

src/themes.cpp

+#include "menu.hpp"
+
+std::string Themes::themedOutput(ull value) {
+  switch (themeCode) {


Ask the question: "If I had 400 themes, will this switch(...) statement be the only method/way of adding a [theme] to [a theme's registry]?".
Is there a more sustainable "generic" method of doing the action of:

[adding an item (or many items) to a list]

[checking if an item exists in the list], and then,

[acting (possibly x command) when an item is found in the list]

https://github.com/BayMinimum/2048.cpp/blob/b9920fe5489c427f9112d2216210de1a86afb82a/src/themes.cpp#L61-L72 enables the contributors to add 'X themes' easily

cawvyoct · 2018-10-17T20:34:50Z

src/headers/themes.hpp

+        elements{"H", "He", "Li", "Be", "B",  "C", "N",
+                 "O", "F",  "Ne", "Na", "Ca", "Sc"},
+        programming_languages{"C",   "C++", "PHP",  "C#", "Py", "Bash", "Java",
+                              "SQL", "CSS", "HTML", "JS", "Go", "Rust"} {}


Ask the question: "If I had 400 themes, is the [Theme's class constructor] the only way a theme can be added to this game?"

It is possible that if it is not sustainable to do so, you may have to make some API to accept:
[some "x theme" datatype] and add it to [a list of some "x theme" datatypes]

Changed: Theme defines just one theme datatype, and ThemeController handles the list of Theme objects

src/themes.cpp

cawvyoct · 2018-10-17T21:43:21Z

For now, the questions asked are very similar, (maybe even related to each other...)
However, solving the challenges one by one so that [question A's] answer doesn't clash with [question B's] answer is the part which may be quite ...frustrating! 😟

However, bit by bit, I think you can do it so you can answer: 😃

each question individually... 🔢
each question in unison... 💯

Questions of course are encouraged! ❔ 😃 👍
So don't be shy to ask / post. This is kinda an 🏔️ 🌊 "iceberg" task so discussions / dialogue will help to get the task complete.

Seperated class Themes into Theme and ThemeController to make it more generic and easily expandable

BayMinimum · 2018-10-18T03:00:21Z

Thanks for your thoughtful comments, @cawvyoct 😃 I've pushed a commit which I believe to solve both problems. Please take a look ✌️

plibither8 · 2018-10-19T04:03:34Z

Hi @BayMinimum, just saw the changes, they look great!

There is one thing that is bugging me 😅. I find it a bit uncomfortable that there are arrays for the possible values, predefined. It seems like we are almost limiting the player from reaching any number or element above the predefined last element of the array. I know it's very rare that a player will ever reach any number above 8192 or the corresponding tiles in the other themes, but we should not limit it to that because of this assumption.

I propose that, instead of having an array, we have a callback function that computes the value of the tile at that index. So for example, if the index is n, the value of the tile should be 2^n (in the normal, classic theme). Similarly, we can compute the nth Fibonacci number or the get the nth element from the Elements array.

BayMinimum · 2018-10-19T05:16:05Z

Hey @plibither8, I really agree to you, and I also had the same uncomfortable feeling 😢 but made the limit since I thought the current implementation of game tiles are enforcing such limit. 🤔 The below is going to happen when number grows beyond 8192:

If such spacing problems can be fixed in future, I'll happily replace the arrays with functions for 'computable' themes!

plibither8 · 2018-10-19T05:43:33Z

Oh! Didn't think about that 😅, you're right, that's definitely an issue! Thanks for #54, we should discuss this there

This patch allows to create themes that has infinitely-long sequence with function, instead of a vector with finite length

BayMinimum · 2018-10-19T07:29:01Z

@plibither8 I've pushed a commit to accept both vectors and functions to create a theme 🍬 Please review those changes when you're free!

plibither8 · 2018-10-19T18:25:48Z

Looks great, thanks a lot 😄!

Just one thing before we merge this:

Right now, just to play a single game, the user has to go through 3 screens to finally start playing: the initial menu, the board-size config and finally choosing the theme. A more easy and simple flow would be to have a fourth option in the settings: "Configuration". Here the user can set the board size and theme. The data can be saved in an external text file (config.txt?) in the data/ directory. When we want to start a new game, the program will read the data in the config file and start the game with those settings, allowing an easier interface for the player. What say @BayMinimum @cawvyoct?

cawvyoct · 2018-10-19T19:07:02Z

Unfortunately a bit busy at the moment but just to say that keep up the good work @BayMinimum as it seems you understood the hints into refactoring your code so it is a bit more flexible than before. 👍

@plibither8

The data can be saved in an external text file (config.txt?) in the data/ directory. When we want to start a new game, the program will read the data in the config file and start the game with those settings, allowing an easier interface for the player.

👍
Agreed, in the end, some sort of external file settings is the way to go.
There are some problems with this however... A sneak peak to the upcoming code review I will reveal would be versioning.
If we change the settings file, which version of the game will it be valid for? The themes challenge is a good one because all these questions need to be considered / answered.

I also see the need for an "infinite play" mode for the game which has values that go beyond the winning tile value however, this is also a challenge when one adds themes.
For example, what is the "infinite play" tile values for the "elements" theme.
Another sneak peak for the code review: What is the winning tile value for the "elements" theme 😉? Is it defined in the theme's "structure"?

This is why I do think that, for now, an "hard-defined" array-based points system is the way to go unfortunately. The game needs to be refactored a bit in its API to accommodate between "defined" and "generated" tile scoring.

This is why I said in the last code review that this was an iceberg-like challenge 😉
It also means that this PR may take a while before its ready for merging IMO. 😿
I do think that @BayMinimum can do this PR though, however the game needs to be updated alongside this PR (being active).

Again, will update soon and leave more feedback. A bit pressed for time at the moment.. 😿

cawvyoct · 2018-11-02T01:00:36Z

Hi @BayMinimum,
Just an update, your themes idea is great! I also liked the way you were going with the design of your Theme data-structure + ThemeController combo.
At the moment, the game is still being refactored quite a bit. 😥 At some point however, you will need to update your PR to match the new refactored code in the codebase.
Would be very grateful if you continued trying to implement theming (when refactoring has settled down) into the project ! 👍
Your contribution has already helped guide things in the project 😸 so hopefully looking forward to more PRs in the near future from you.

added: theme feature

9efb8a2

Added Themes class to handle theming and applied to game

cawvyoct reviewed Oct 17, 2018

View reviewed changes

src/themes.cpp Outdated Show resolved Hide resolved

fix: seperate Theme and ThemeController

b9920fe

Seperated class Themes into Theme and ThemeController to make it more generic and easily expandable

plibither8 changed the title ~~added: theme feature~~ Feature: Implements 'themes' Oct 19, 2018

Enhancement: make Theme constructor accept function

895ed24

This patch allows to create themes that has infinitely-long sequence with function, instead of a vector with finite length

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Implements 'themes' #52

Feature: Implements 'themes' #52

BayMinimum commented Oct 12, 2018

cawvyoct commented Oct 15, 2018

BayMinimum commented Oct 15, 2018

plibither8 commented Oct 16, 2018

cawvyoct commented Oct 17, 2018 •

edited

Loading

cawvyoct commented Oct 17, 2018

cawvyoct commented Oct 17, 2018 •

edited

Loading

cawvyoct Oct 17, 2018

BayMinimum Oct 18, 2018

cawvyoct Oct 17, 2018

BayMinimum Oct 18, 2018

cawvyoct commented Oct 17, 2018

BayMinimum commented Oct 18, 2018

plibither8 commented Oct 19, 2018

BayMinimum commented Oct 19, 2018

plibither8 commented Oct 19, 2018

BayMinimum commented Oct 19, 2018

plibither8 commented Oct 19, 2018

cawvyoct commented Oct 19, 2018

cawvyoct commented Nov 2, 2018

Feature: Implements 'themes' #52

Are you sure you want to change the base?

Feature: Implements 'themes' #52

Conversation

BayMinimum commented Oct 12, 2018

cawvyoct commented Oct 15, 2018

BayMinimum commented Oct 15, 2018

plibither8 commented Oct 16, 2018

cawvyoct commented Oct 17, 2018 • edited Loading

cawvyoct commented Oct 17, 2018

cawvyoct commented Oct 17, 2018 • edited Loading

cawvyoct Oct 17, 2018

Choose a reason for hiding this comment

BayMinimum Oct 18, 2018

Choose a reason for hiding this comment

cawvyoct Oct 17, 2018

Choose a reason for hiding this comment

BayMinimum Oct 18, 2018

Choose a reason for hiding this comment

cawvyoct commented Oct 17, 2018

BayMinimum commented Oct 18, 2018

plibither8 commented Oct 19, 2018

BayMinimum commented Oct 19, 2018

plibither8 commented Oct 19, 2018

BayMinimum commented Oct 19, 2018

plibither8 commented Oct 19, 2018

cawvyoct commented Oct 19, 2018

cawvyoct commented Nov 2, 2018

cawvyoct commented Oct 17, 2018 •

edited

Loading

cawvyoct commented Oct 17, 2018 •

edited

Loading