Research developement

[michal]

I can't clone it too (using git command under linux).

Maybe you have some closed ports for git?

[gchevallereau]

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.

[SupSuper]

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. 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.
- 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.

[gchevallereau]

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.

[darko]

guys.. guys.. dont argue 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..
=)

[michal]

guys.. guys.. dont argue 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.

[SupSuper]

guys.. guys.. dont argue 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.

[gchevallereau]

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.

[SupSuper]

Research branch is now in the official codebase, news post to come soon.

[darko]

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  

[michal]

Is research fully finished? What about manufacture?

[luke83]

Wow research is in code base ,  WOW This project is progressing so rapidly   

I wish the developers at Moo2 ( MASTER OF ORION OPEN SOURCE) was moving this quick.

[gchevallereau]

Is research fully finished? What about manufacture?

Yes research is finished. I have already started(and almost finished) manufacture.

[michal]

Cool I hope it soon also will be merged. Maybe 0.4 will be released faster than 0.3

[Daiky]

oh no, more pressure on me now