Multiple things done - Githubissues

amtyurin commented 3 years ago

what done:

Split files with several classes definitions into separate files
Scenario is loaded for main game
Editor can be used to edit scenario (change terrain)
Can save/load scenario
fixed: if do normal exit the next start is failed
Terrain images are used instead of monotonic colors
Replaced yaml by pickle so Yaml is not needed anymore

Trilarion commented 3 years ago

Thanks, I will look at it.

Trilarion commented 3 years ago

The single commit (https://github.com/amtyurin/imperialism-remake/commit/c605d373998db670ceebd5d4dc5b9a68d0491e64) contains a lot of changes in only one commit. It's a bit difficult to get a good overview over all the changes quickly. Separating the changes in multiple commits would probably be better (Python class splitting = 1 commit, terrain addition = 1 commit, ...).

Here are some questions:

Why splitting the Python code in a single class per file? What are the advantages? IDEs can navigate easily in large files and keeping related classes together also has advantages. (see also https://stackoverflow.com/questions/106896/how-many-classes-should-i-put-in-one-file)
Why using ushahidi_sphinx_rtd_theme? Have you experience with it? It seems to be not very popular (https://github.com/ushahidi/ushahid_sphinx_rtd_theme) and outdated (last updated 2015 and many commits behind).
Why using pickle instead of yaml? Reducing a dependency is good, but the advantage of yaml are human readable files. The scenario file but for example also the soundtrack.info has become unreadable with a text editor. With yaml all these could be edited also manually if needed.
Adding the old terrain resources is good and immediately improves the look. Added license information would still be needed though. I could do it in this case. In general no artwork should be commited without copyright information.
There are other things like fixes here and there that are undisputed. Would it be okay, if I already cherry-pick them and apply them to here?

amtyurin commented 3 years ago

Hello,

I agree that it is difficult, but I am doing a lot of changes while the source base is small yet. In future I will split commits logically. I hope to do that while no-one is doing any changes.

Please find the answers:

Why splitting the Python code in a single class per file? What are the advantages? IDEs can navigate easily in large files and keeping related classes together also has advantages. (see also https://stackoverflow.com/questions/106896/how-many-classes-should-i-put-in-one-file) [AT] To make it easier to change them by many people in parallel. I’ve tried to split them logically but it is not the final split yet. A lot of work needs to be done there because it is only start (of the project)
Why using ushahidi_sphinx_rtd_theme? Have you experience with it? It seems to be not very popular (https://github.com/ushahidi/ushahid_sphinx_rtd_theme) and outdated (last updated 2015 and many commits behind). [AT] You are right here. I could not find sphinx_rtd_theme in PyCharm package repository and used that instead, may be I need to add another repo link where it is located. I will check that.
Why using pickle instead of yaml? Reducing a dependency is good, but the advantage of yaml are human readable files. The scenario file but for example also the soundtrack.info has become unreadable with a text editor. With yaml all these could be edited also manually if needed. [AT] The main problem with Yaml is that it does not store/read non standard Python types and it is necessary to convert them to plain ones. Pickle can just serialize anything and read anything. E.g. that eliminated problem with saving/reading options file when QRect object was used. The question is how often do we need to change configs manually rather than change them in the code. We can set protocol=0 for it to make configs human readable, but it will not help much.
Adding the old terrain resources is good and immediately improves the look. Added license information would still be needed though. I could do it in this case. In general no artwork should be commited without copyright information. [AT] Yes, I got the images from old project and looks like forgot to copy license files.
There are other things like fixes here and there that are undisputed. Would it be okay, if I already cherry-pick them and apply them to here? [AT] I agree that it is hard because of many changes. Moreover I have more of them upcoming, so it would be better to wait and you can pick some of them you like more :)

It is always hard to make stable things from the beginning and the structure will be changed in any case until there is some backbone/core.

Let me finish some stuff till the end of this week so you can pick most useful and clean stuff.

Thank you for your feedback!

Thanks, Andrey

On Sep 1, 2020, at 6:00 AM, Trilarion notifications@github.com wrote:

The single commit (amtyurin@c605d37 https://github.com/amtyurin/imperialism-remake/commit/c605d373998db670ceebd5d4dc5b9a68d0491e64) contains a lot of changes in only one commit. It's a bit difficult to get a good overview over all the changes quickly. Separating the changes in multiple commits would probably be better (Python class splitting = 1 commit, terrain addition = 1 commit, ...).

Here are some questions:

Why splitting the Python code in a single class per file? What are the advantages? IDEs can navigate easily in large files and keeping related classes together also has advantages. (see also https://stackoverflow.com/questions/106896/how-many-classes-should-i-put-in-one-file https://stackoverflow.com/questions/106896/how-many-classes-should-i-put-in-one-file)

Why using ushahidi_sphinx_rtd_theme? Have you experience with it? It seems to be not very popular (https://github.com/ushahidi/ushahid_sphinx_rtd_theme https://github.com/ushahidi/ushahid_sphinx_rtd_theme) and outdated (last updated 2015 and many commits behind).

Why using pickle instead of yaml? Reducing a dependency is good, but the advantage of yaml are human readable files. The scenario file but for example also the soundtrack.info has become unreadable with a text editor. With yaml all these could be edited also manually if needed.

Adding the old terrain resources is good and immediately improves the look. Added license information would still be needed though. I could do it in this case. In general no artwork should be commited without copyright information.

There are other things like fixes here and there that are undisputed. Would it be okay, if I already cherry-pick them and apply them to here?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Trilarion/imperialism-remake/pull/34#issuecomment-684834489, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXCT4FA2WDG7QHFPDSAS4LSDTV7FANCNFSM4QP2473A.

Trilarion commented 3 years ago

Hi Andrey,

Why splitting the Python code in a single class per file?: To make it easier to change them by many people in parallel. I think that this is a minor problem. If files are edited in parallel by multiple people but at different places in a file, the conflicts can be solved trivially by git itself. Only if they happen at the same code positions in a file, the conflict would be needed to be solved manually and this is the same if classes are in one file or distributed over many files. Maybe you come from the Java world where a single class per file is strongly recommended.

I don't really see an advantage, but also not many disadvantages except for more import statements needed in every additional file, and you seem to be quite motivated to do something so if you like single class files, we should do that.

Why using ushahidi_sphinx_rtd_theme? I could not find sphinx_rtd_theme in PyCharm package repository and used that instead PyCharm uses either conda and the Anaconda package repository or pip and PyPI. At least on PyPI, there is the sphinx_rtd_theme (https://pypi.org/project/sphinx-rtd-theme/).

Why using pickle instead of yaml? The main problem with Yaml is that it does not store/read non standard Python types and it is necessary to convert them to plain ones. Pickle can just serialize anything and read anything. That's indeed a big problem and I was not really aware of that. I think that some configurations like for example the soundtrack playlist will rather be edited manually while others like the scenarios or the network messages will only edited by the application itself like scenarios.

I guess we can do without yaml. We can use pickle for all stuff that is not edited manually or contains non standard Python types and for simple configurations like the playlist we can use JSON which is included in Python. I can probably use your commit and make a derived commit out of it that does this. The advantage would be that configurations can be changed much easier also by non-programmers and JSON is still very much readable.

Adding the old terrain resources..I got the images from old project I can add the license information.

I agree that it is hard because of many changes. Moreover I have more of them upcoming, so it would be better to wait ... Let me finish some stuff till the end of this week so you can pick most useful and clean stuff. OK. I can integrate it on the weekend and I can already start with the commits you have now. I will probably divide your big commit into multiple ones. And then maybe we can discuss a bit how the project should be continued. Feel free to contribute in any way you see sense. I will mostly check pull requests for consistency and that they also run on my system.

amtyurin commented 3 years ago

Why splitting the Python code in a single class per file?: To make it easier to change them by many people in parallel. I think that this is a minor problem. If files are edited in parallel by multiple people but at different places in a file, the conflicts can be solved trivially by git itself. Only if they happen at the same code positions in a file, the conflict would be needed to be solved manually and this is the same if classes are in one file or distributed over many files. Maybe you come from the Java world where a single class per file is strongly recommended.

I don't really see an advantage, but also not many disadvantages except for more import statements needed in every additional file, and you seem to be quite motivated to do something so if you like single class files, we should do that.

[AT] I do not mind combining classes into one file if they are used only that file. But if that class is used in many classes in different files it would be better to move it to separate file. Anyway, please feel free combine classes into a file.

Why using ushahidi_sphinx_rtd_theme? I could not find sphinx_rtd_theme in PyCharm package repository and used that instead PyCharm uses either conda and the Anaconda package repository or pip and PyPI. At least on PyPI, there is the sphinx_rtd_theme (https://pypi.org/project/sphinx-rtd-theme/). [AT] I’ll change that back, MY PyCharm did not see that package, so I will install it manually using pip in CLI

Why using pickle instead of yaml? The main problem with Yaml is that it does not store/read non standard Python types and it is necessary to convert them to plain ones. Pickle can just serialize anything and read anything. That's indeed a big problem and I was not really aware of that. I think that some configurations like for example the soundtrack playlist will rather be edited manually while others like the scenarios or the network messages will only edited by the application itself like scenarios.

I guess we can do without yaml. We can use pickle for all stuff that is not edited manually or contains non standard Python types and for simple configurations like the playlist we can use JSON which is included in Python. I can probably use your commit and make a derived commit out of it that does this. The advantage would be that configurations can be changed much easier also by non-programmers and JSON is still very much readable.

[AT] As we use python we can use .py as configuration file and we can get rid of redundant config files. What do you think? E.g. we can create separate folder for configuration files .py and we can just load them without any complications

Adding the old terrain resources..I got the images from old project I can add the license information. [AT] I did this already

I agree that it is hard because of many changes. Moreover I have more of them upcoming, so it would be better to wait ... Let me finish some stuff till the end of this week so you can pick most useful and clean stuff. OK. I can integrate it on the weekend and I can already start with the commits you have now. I will probably divide your big commit into multiple ones. And then maybe we can discuss a bit how the project should be continued. Feel free to contribute in any way you see sense. I will mostly check pull requests for consistency and that they also run on my system.

[AT] Yes. May be we can discuss the common approach, because I do not quite understand client<->server communication now, so I concentrated only on UI/client side and model creation.

I am going to finish major changes this Friday/Sunday.

Andrey

On Sep 2, 2020, at 3:38 AM, Trilarion notifications@github.com wrote:

Why splitting the Python code in a single class per file?: To make it easier to change them by many people in parallel. I think that this is a minor problem. If files are edited in parallel by multiple people but at different places in a file, the conflicts can be solved trivially by git itself. Only if they happen at the same code positions in a file, the conflict would be needed to be solved manually and this is the same if classes are in one file or distributed over many files. Maybe you come from the Java world where a single class per file is strongly recommended.

I don't really see an advantage, but also not many disadvantages except for more import statements needed in every additional file, and you seem to be quite motivated to do something so if you like single class files, we should do that.

Why using ushahidi_sphinx_rtd_theme? I could not find sphinx_rtd_theme in PyCharm package repository and used that instead PyCharm uses either conda and the Anaconda package repository or pip and PyPI. At least on PyPI, there is the sphinx_rtd_theme (https://pypi.org/project/sphinx-rtd-theme/ https://pypi.org/project/sphinx-rtd-theme/).

Why using pickle instead of yaml? The main problem with Yaml is that it does not store/read non standard Python types and it is necessary to convert them to plain ones. Pickle can just serialize anything and read anything. That's indeed a big problem and I was not really aware of that. I think that some configurations like for example the soundtrack playlist will rather be edited manually while others like the scenarios or the network messages will only edited by the application itself like scenarios.

I guess we can do without yaml. We can use pickle for all stuff that is not edited manually or contains non standard Python types and for simple configurations like the playlist we can use JSON which is included in Python. I can probably use your commit and make a derived commit out of it that does this. The advantage would be that configurations can be changed much easier also by non-programmers and JSON is still very much readable.

Adding the old terrain resources..I got the images from old project I can add the license information.

I agree that it is hard because of many changes. Moreover I have more of them upcoming, so it would be better to wait ... Let me finish some stuff till the end of this week so you can pick most useful and clean stuff. OK. I can integrate it on the weekend and I can already start with the commits you have now. I will probably divide your big commit into multiple ones. And then maybe we can discuss a bit how the project should be continued. Feel free to contribute in any way you see sense. I will mostly check pull requests for consistency and that they also run on my system.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Trilarion/imperialism-remake/pull/34#issuecomment-685591260, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXCT4ELH5FWD55R4AYEXF3SDYOCHANCNFSM4QP2473A.

Trilarion commented 3 years ago

"As we use python we can use .py as configuration file and we can get rid of redundant config files. What do you think? E.g. we can create separate folder for configuration files .py and we can just load them without any complications"

That's another possibility, but these configuration Python files would basically not be able to contain any real Python code to be able to be edited by non-programmers and then they are equivalent to JSON or any other format more or less. I'm not sure that's a common approach to use Python code directly for configurations.

"Yes. May be we can discuss the common approach, because I do not quite understand client<->server communication now, so I concentrated only on UI/client side and model creation. I am going to finish major changes this Friday/Sunday."

I will wait for your changes, then integrate them and then write something about the client-server communication.

Trilarion commented 3 years ago

A short search on configuration files in Python brought https://martin-thoma.com/configuration-files-in-python/ or https://hackingandslacking.com/simplify-your-python-project-configuration-e0a7699fb77c and it boils down to either configuration Python scripts or JSON and it really doesn't matter which one of them.

amtyurin commented 3 years ago

I’ve returned sphinx_* lib usage. Also, added workforce classes with 2 types (Engineer, Geologist), no screens for them right now (I used army icon for stand and action icons) and possibility to move on the ground. It is not completed yet. Do you know any free resources for icons/images? I would like to take to take some.

About config. We can use something like that: Config_blah.py setting1 = “some setting” setting2= { ‘field1’: ‘value1’, …..} setting3 = 123

Example is client/config/config.py for logging

On Sep 4, 2020, at 5:34 AM, Trilarion notifications@github.com wrote:

A short search on configuration files in Python brought https://martin-thoma.com/configuration-files-in-python/ https://martin-thoma.com/configuration-files-in-python/ or https://hackingandslacking.com/simplify-your-python-project-configuration-e0a7699fb77c https://hackingandslacking.com/simplify-your-python-project-configuration-e0a7699fb77c and it boils down to either configuration Python scripts or JSON and it really doesn't matter which one of them.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Trilarion/imperialism-remake/pull/34#issuecomment-687115192, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXCT4FJ4CID2TXX7C3XSADSEDNFLANCNFSM4QP2473A.

Trilarion commented 3 years ago

There is for example https://opengameart.org/ but they may not have items for these classes and it's difficult to get a coherent style. What is needed in the long run is an artist who can produce a coherent set of images.

I'm integrating it. I'm not yet convinced that configuration Python files is really the better solution.

Andrettin commented 3 years ago

On art, in case it's helpful, I commissioned some graphics for a grand strategy project for craftsmen, which you could use for engineers: https://raw.githubusercontent.com/Andrettin/Iron-Barons/master/graphics/icons/population/craftsmen.png

The slave graphics could be used as a farmer, too: https://raw.githubusercontent.com/Andrettin/Iron-Barons/master/graphics/icons/population/slaves.png

There is a bit more here, with skin color variations: https://github.com/Andrettin/Iron-Barons/tree/master/graphics/icons/population

They are all licensed under the CC0, and were made by Jinn (from whom I commissioned the graphics).

amtyurin commented 3 years ago

OK, Thanks. I will check those.

On Sep 6, 2020, at 2:48 AM, Andrettin notifications@github.com wrote:

On art, in case it's helpful, I commissioned some graphics for a grand strategy project for craftsmen, which you could use for engineers: https://raw.githubusercontent.com/Andrettin/Iron-Barons/master/graphics/icons/population/craftsmen.png https://raw.githubusercontent.com/Andrettin/Iron-Barons/master/graphics/icons/population/craftsmen.png The slave graphics could be used as a farmer, too: https://raw.githubusercontent.com/Andrettin/Iron-Barons/master/graphics/icons/population/slaves.png https://raw.githubusercontent.com/Andrettin/Iron-Barons/master/graphics/icons/population/slaves.png There is a bit more here, with skin color variations: https://github.com/Andrettin/Iron-Barons/tree/master/graphics/icons/population https://github.com/Andrettin/Iron-Barons/tree/master/graphics/icons/population They are all licensed under the CC0, and were made by Jinn (from whom I commissioned the graphics).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Trilarion/imperialism-remake/pull/34#issuecomment-687741989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXCT4FHEY3JSMKUBMHLT5TSENLFBANCNFSM4QP2473A.

Trilarion commented 3 years ago

I resolved the initial conflict and added all your recent commits from your fixes branch. This is from your master branch which is behind your fixes branch and now also my master branch. This branch does not need to be merged anymore.

Trilarion / imperialism-remake

Multiple things done #34