OpenXcom Forum
Contributions => Programming => Topic started by: gchevallereau on August 26, 2011, 06:34:40 pm
-
Hi,
Over the last weeks, I have tried to implement research in OpenXcom. It's now working and I would welcome feedback on it.
What's need to be done:
* Documentation(especially the Dependency/unlock parts).
* Minor bug : If no project can be listed when Clicking on New project, you will be back the base view.
* UI bug : Colours are wrong in the Project Setting State(and sometimes in the End Research State).
* Research cost : According to https://www.ufopaedia.org/index.php?title=Research_Technical_Details, Research cost have +-50% cost modifier. It should be to easy to add it.
I have also added a CMake build to OpenXcom. I have checked that it work on Linux and Windows(MinGW build). It should also work for Visual Studio.
You can get the code from my personal git repo here: https://gchevallereau.net/cgit/cgit.cgi/OpenXcom
Code is in the research_cmake branch. The cmake branch hold the CMake build system port.
-
Wow
-
Wow
Seconded. :o
I'll be sure to have a look and give you some feedback after 0.3 is out. If you're looking to get it integrated into the main OpenXcom codebase, my only suggestion right now is to check the guidelines and make sure it meshes in well with the rest of the code.
-
That's great! ;D
I did look a bit at the code, but couldn't find out how you manage to finish a research.
However, you can link this event to unlock certain articles in Ufopedia, by calling
Ufopaedia::releaseArticle(Game *game, std::string &article_id);
Or, to open the corresponding article state: Ufopaedia::open(...);
See Ufopaedia.h for details.
-
Hi,
my only suggestion right now is to check the guidelines and make sure it meshes in well with the rest of the code
I have tried to follow the coding rules, but maybe I have missed something. I will check that. By the way, maybe it would be helpfull to have something like https://astyle.sourceforge.net/astyle.html (https://astyle.sourceforge.net/astyle.html) to check this ?
I did look a bit at the code, but couldn't find out how you manage to finish a research.
It's in the GeoscapeState::time1Day function (https://gchevallereau.net/cgit/cgit.cgi/OpenXcom/tree/src/Geoscape/GeoscapeState.cpp?h=research_cmake#n751 (https://gchevallereau.net/cgit/cgit.cgi/OpenXcom/tree/src/Geoscape/GeoscapeState.cpp?h=research_cmake#n751)).
Or, to open the corresponding article state: Ufopaedia::open(...);
Already done : https://gchevallereau.net/cgit/cgit.cgi/OpenXcom/tree/src/Geoscape/EndResearchState.cpp?h=research_cmake#n84 (https://gchevallereau.net/cgit/cgit.cgi/OpenXcom/tree/src/Geoscape/EndResearchState.cpp?h=research_cmake#n84). But the openArticle is not yet implemented. By the way, I think that the std::string & should be a const one.
However, you can link this event to unlock certain articles in Ufopedia, by calling
Ufopaedia::releaseArticle(Game *game, std::string &article_id);
I will add this.
-
So gchevallereau, now you will implement manufacture and geoscape for 0.4 will be almost ready? ;)
-
now you will implement manufacture
Well I was thinking to it. But it would be better to wait for feedback on the research as manufacture will depend on it.
-
Hi,
A little update on the research branch. Here is the current status:
* Documentation(especially the Dependency/unlock parts). I have added doxygen documentation to most class. Not sure if it's enough
* Minor bug : If no project can be listed when Clicking on New project, you will be back the base view. FIXED
* UI bug : Colours are wrong in the Project Setting State(and sometimes in the End Research State). Partially fixed. Colors are much more better now. But there still some little problems(assigned scientist, used space lab, available sicentist in the ResearchProjectState still have the wrong colors).
* Research cost : According to https://www.ufopaedia.org/index.php?title=Research_Technical_Details, Research cost have +-50% cost modifier. It should be to easy to add it. DONE
* However, you can link this event to unlock certain articles in Ufopedia, by calling
Ufopaedia::releaseArticle(Game *game, std::string &article_id);
DONE
I reviewed most of the modification and try to make sure that it follow coding rules.
-
Your code looks great. I noticed you use relative coordinates to position your UI and various variables for the numbers, instead of absolute coordinates and hardcoded numbers, that's good :)
-
Your code looks great. I noticed you use relative coordinates to position your UI and various variables for the numbers, instead of absolute coordinates and hardcoded numbers, that's good :)
Let me guess - you're thinking about higher resolutions? ;)
-
Let me guess - you're thinking about higher resolutions? ;)
You probably don't want to know what I'm thinking :)
-
btw gchevallereau, will you be introducing to us eventually? :)
Also we don't know what your plans are, and you don't know what our plans are. It's a bit an awkward situation to be honest. I'm already starting to get scared you will take away the fun parts of battlescape core development too :p
-
btw gchevallereau, will you be introducing to us eventually?
Well, I'm french 30 years developer. I have discovered openxcom only 2 month ago. As I'm at the moment looking for a job, I have some time to spent on code.
Also we don't know what your plans are, and you don't know what our plans are.
You mean that the roadmap is not accurate ? ;D
More seriously, I'm just looking at the research/manufacture code at the moment. Nothing more.
I'm already starting to get scared you will take away the fun parts of battlescape core development too :p
Don't worry ;) I promise I won't touch it.
-
Since you're from europe too, maybe you could enter our irc channel ( details are here https://openxcom.org/forum/index.php?topic=63.0 ) ? SupSuper and Daiky are usually at evenings.
-
Also we don't know what your plans are, and you don't know what our plans are.
You mean that the roadmap is not accurate ? ;D
More seriously, I'm just looking at the research/manufacture code at the moment. Nothing more.
Well we can have things planned out in our heads before they're coded. I feel like I've been demoted. :P j/k
Anyways I tried to clone your Git repository to have a better look at your code but I get this error:
"Cloning into D:\SupFiles\Visual Studio Projects\OpenXcom\research...
gchevallereau.net[0: 88.169.205.14]: errno=No error
fatal: unable to connect a socket (No error)"
Any ideas?
-
I can't clone it too (using git command under linux).
Maybe you have some closed ports for git?
-
Anyways I tried to clone your Git repository to have a better look at your code but I get this error:
"Cloning into D:\SupFiles\Visual Studio Projects\OpenXcom\research...
gchevallereau.net[0: 88.169.205.14]: errno=No error
fatal: unable to connect a socket (No error)"
Any ideas?
Oops! We made some change to our internet connection recently and I forgot to update the firewall rules. Anyway, it's fixed now.
-
Ok I've looked over your code, even though every time I look at someone else's code I have an urge to nuke it from orbit. :P j/k I'm sure every programmer feels the same way.
Here's some feedback:
- You don't need to keep around pointers to various states to update them, they have a init() function that's called whenever the State is focused again, which you can use for updating contents and restoring palettes.
- Geoscape windows should use the GeoscapeState::popup function instead of the Game pushState, it handles multiple popups showing up at once.
- The code could use some cleanup. There's functions spread around in the global namespace which belong elsewhere, like the getFreeLabSpace and getFreeScientist in ResearchProjectState. There's also various leftovers from the code you copied like unused includes and such.
- The UI is also a bit messy, some text and buttons not matching the original, though if it's not your thing I can fix it up later.
- Also the UI is completely static, so I don't think you need a lot of calculations like you did in ResearchProjectState.
- What's the findRuleResearchProjectByString in RuleResearchProject for? I've never even seen a std::unary_function before. :P
- Why are the ResearchProject costs floats? From what I could gather in Ufopaedia.org they should all be integer values, and in RuleResearchProject it's an integer too.
- Regarding Doxygen comments, we usually put comments at the start of every class and function. You seem to have covered everything except your new States.
As for the research functionality itself, it seems fine and it's nice to see it's already externalized. Mind you if the research projects already have dependencies, do they really need the unlocks? It would cause problems with things that require multiple researches. I was thinking it might be simpler just keep a list of unlocked researches per savegame and have everything else have research dependencies, instead of having to keep lists of unlocks for everything.
I will have a look at that "astyle" program to bring everything up to the same style.
-
Hi,
- You don't need to keep around pointers to various states to update them, they have a init() function that's called whenever the State is focused again, which you can use for updating contents and restoring palettes.
FIXED : That much more simpler that way.
- Geoscape windows should use the GeoscapeState::popup function instead of the Game pushState, it handles multiple popups showing up at once.
FIXED
- The code could use some cleanup. There's functions spread around in the global namespace which belong elsewhere, like the getFreeLabSpace and getFreeScientist in ResearchProjectState
I assume you would like to see them in the Base class ?
There's also various leftovers from the code you copied like unused includes and such.
I will try to remove them.
- The UI is also a bit messy, some text and buttons not matching the original, though if it's not your thing I can fix it up later.
I remember that the NewResearchState is a little bit different, but that the only one I an think of. Are there any other one ?
Speaking of that, I have tried to follow the original Xcom, but there are place where UI could be better. Especially the EndResearchState. You can get 3 different window ("Research complete", "We can now research", "We can now produce"). Might be better to merge all of them in one window ?
- Also the UI is completely static, so I don't think you need a lot of calculations like you did in ResearchProjectState.
You mean the placement of widget in the window ? Well it's just that i feel this easier when creating the window.
- What's the findRuleResearchProjectByString in RuleResearchProject for? I've never even seen a std::unary_function before.
They are helper struct used for std::find_if algorithm. I have seen that instead, you mainly use std::map. I can change my code to use a std::map, if you prefer it that way.
- Why are the ResearchProject costs floats? From what I could gather in Ufopaedia.org they should all be integer values, and in RuleResearchProject it's an integer too.
FIXED:My bad. At first reading, I have guessed that they should be float. I have changed them to integer
- Regarding Doxygen comments, we usually put comments at the start of every class and function. You seem to have covered everything except your new States.
Yes, I was not sure if states really need it. I can add them if needed.
Mind you if the research projects already have dependencies, do they really need the unlocks? It would cause problems with things that require multiple researches. I was thinking it might be simpler just keep a list of unlocked researches per savegame and have everything else have research dependencies, instead of having to keep lists of unlocks for everything.
Multiple dependencies are already covered. Unlocks are needed because there are some corner case. For example Psi-lab can be unlocked by differents research. I agree that it's not the best solution, but I don't really understand your proposition.
-
guys.. guys.. dont argue :P i'm nobody to tell y and little man here, but y all doing great job!
i think that express most guys opinions who read this topic..
maybee give to gchevallereau access to openXcom trunk? he works like pro and quite hard, just little imho..
=)
-
guys.. guys.. dont argue :P i'm nobody to tell y and little man here, but y all doing great job!
i think that express most guys opinions who read this topic..
maybee give to gchevallereau access to openXcom trunk? he works like pro and quite hard, just little imho..
=)
Don't worry Darko, nothing bad happens ;) They just discussing how to do things in best way.
-
guys.. guys.. dont argue :P i'm nobody to tell y and little man here, but y all doing great job!
i think that express most guys opinions who read this topic..
maybee give to gchevallereau access to openXcom trunk? he works like pro and quite hard, just little imho..
=)
It's just constructive criticism, the better it integrates with codebase the sooner it'll be added. :)
-
Hi,
A little update on the state of the branch.
The following has been fixed:
- The code could use some cleanup. There's functions spread around in the global namespace which belong elsewhere, like the getFreeLabSpace and getFreeScientist in ResearchProjectState
- The code could use some cleanup. There's functions spread around in the global namespace which belong elsewhere, like the getFreeLabSpace and getFreeScientist in ResearchProjectState
- What's the findRuleResearchProjectByString in RuleResearchProject for? I've never even seen a std::unary_function before.
- Regarding Doxygen comments, we usually put comments at the start of every class and function. You seem to have covered everything except your new States.
There's also various leftovers from the code you copied like unused includes and such.
SupSuper also point me to a problem with ResearchDependency. The plasma weapons tree described here : https://www.ufopaedia.org/index.php?title=Research_Trees was not possible. Fortunately, we have found a solution and it's now implemented.
guys.. guys.. dont argue Tongue i'm nobody to tell y and little man here, but y all doing great job!
i think that express most guys opinions who read this topic..
Don't worry, nobody is arguing here. As Daiky and SupSuper have explained we are simply discussing what need to be done to see this merged to the main OpenXcom branch.
-
Research branch is now in the official codebase, news post to come soon. :)
-
cool news, project is progressing quite fast .. btw looks like production branch is also mostly done... after it its possible to say all root stuff completed :P
-
Is research fully finished? What about manufacture?
-
Wow research is in code base , WOW This project is progressing so rapidly :o
I wish the developers at Moo2 ( MASTER OF ORION OPEN SOURCE) was moving this quick.
-
Is research fully finished? What about manufacture?
Yes research is finished. I have already started(and almost finished) manufacture.
-
Cool :) I hope it soon also will be merged. Maybe 0.4 will be released faster than 0.3 :D
-
oh no, more pressure on me now :)
-
Don't worry I'll be sure to drag it out until everyone comes to my house with torches and pitchforks. :P
-
Don't worry I'll be sure to drag it out until everyone comes to my house with torches and pitchforks. :P
.. or maybe they bring champagne for the release party? ;)
-
... or maybe stun rods? :D I think SupSuper could have connection to aliens :>
-
gchevallereau, what about integrating research with ufopaedia? It would be nice if ufopeadia could show researched things. For now ufopedia shows couple of example articles, research does not unlock new articles.
-
gchevallereau, what about integrating research with ufopaedia? It would be nice if ufopeadia could show researched things. For now ufopedia shows couple of example articles, research does not unlock new articles.
It is integrated, the problem is the new articles don't exist yet (and they'd also need the new items, facilities, etc. to tie into). I'm hoping to get around to this once the rulesets are in place so we can easily put all the info in instead of just the "starting stuff".
-
I have a suggestion, is it hard to make the prerequisite items not just a flag, but a list of items? It would make it a lot more flexible to potential mods to the research tree.
So just looking at the research.dat:
- name: STR_PLASMA_RIFLE
cost: 700
needItem: true
dependencys:
[]
unlock:
[]
would become:
- name: STR_PLASMA_RIFLE
cost: 700
prereqItems:
- STR_PLASMA_RIFLE
prereqResearch:
[]
unlock:
[]
-
Interesting. So we would be able to merge weapon+clip researches. It is annoying to research rifles and clips separately.
-
It should be really simple. The needed modifications should be:
* Transform the bool flag to a list in RuleResearchProject
* Change the SavedGame::getAvailableResearchProjects function to check a list of items instead of just one.
If we do it, it might even be interresting to set a number of needed items, and maybe if they should be consumed. We can image that scientist need to disasemble items to understand it. So your example could become:
- name: STR_PLASMA_RIFLE
cost: 700
prereqItems:
- STR_PLASMA_RIFLE:
- needed: 3
- destroyed: true
prereqResearch:
[]
unlock:
[]
-
that would actually be really cool. just remember to tell the player what's going to happen on the research screen :) something like:
New Research: <alien artifact name>
With the recovered <alien artifact name> we should now be capable of reverse-engineering it. The research will use <x> unit(s) of <alien artifact name><, which will be destroyed in the process>.
You will also have to decide whether or not the research will become visible before all requirements are met. Just imagine a player having a Plasma Rifle in his stores and wondering why he is unable to research it. I for example always sold all alien artifacts but one if I was fairly certain that I don't need them... so maybe another line should appear in that case:
The stores currently hold <y> <alien artifact name>(s). In order to start the research, we need <x> units.
-
IMO the number of needed items can be coded in, but should not be part of the stock OpenXcom. It is a feature that's included for modders to use if they wish.