OpenXcom Forum
Contributions => Programming => Topic started by: karvanit on November 09, 2012, 01:24:35 am
-
I have the following questions about some of the decisions in OpenXcom code:
- Why are we not using YAML anchors and aliases instead of thesaveId() functions? The file structure will be cleaner, and so will the code.
- Instead of creating the objects and then loading them, why don't we have a "loading" constructor? Again, the code will be cleaner, since we can remove some functions that only get used once.
- Why don't we have the Game object as a global object (or Singleton)? It is required in LOTS of places and we just store and pass it around (or it's Ruleset and SavedGame members, which amounts to the same thing). Making it visible everywhere would simplify LOTS of code and remove redundant arguments from lots of functions. It's not like we will ever have more than one.
- Since we gave get/set for every member in some classes, why not make them plain structs?
- Why do we return pointers to collections (eg Game::getUfos()) instead of references? The code has a lot of places where references are more suited to what we mean instead of pointers (eg Everywhere we store Game, Rule* and SavedGame pointers in classes as fields).
- Why do we return non-const rules? They should not have writable fields after they are initialized.
If the answer is "too much work to change", I am more than willing to make a patch for either of the above.
-
Why don't we have the Game object as a global object (or Singleton)? It is required in LOTS of places and we just store and pass it around (or it's Ruleset and SavedGame members, which amounts to the same thing). Making it visible everywhere would simplify LOTS of code and remove redundant arguments from lots of functions. It's not like we will ever have more than one.[/li][/list]
Having global variables is pretty much defying to OOP's principles. Even if it would be simpler. :)
-
So OOP principles work on complication? Well, pragmatic approach defies any principle that implies any complication. OOP is not some holy cow to idolize it.
-
Actually it's not me who adheres to that principle, but the OpenXcom coding style does (https://www.ufopaedia.org/index.php?title=Coding_Style_(OpenXcom)).
-
Classes having their data blindly manipulated from others directly is not OOP.
OOP means having objects represent ideas and keeping interactions to a minimum, each object containing all the data it needs and preneting the actions that make sense. So, for example, the Ufo should present methods for moving, damaging it or checking for interesting conditions (destroyed, landed, moving). Not give free access to everyone to directly set member variables. Moving all moving things on the map should be the responsibility of the class holding the list of all movables on the map, etc for many things.
Our Game object now has 3 distinct roles. It's the main loop, the game state database and the rules database.
The main loop is a service and should be a global object.
Many things require the ruleset, and we only ever care for one, so a global ruleset is not unreasonable.
Since our objects don't have a clean state separation (data and behaviour are split across many objects), we need to pass the savegame around. But that is because it too has two roles. It's the 'persistent data' and also the 'current game state of all objects'. The 'current game state' is also a candidate for a global object, to be used everywhere except load/save routines (which actually care for the persistent data).
The above are my personal opinion and are not gospel. But I am obviously biased to think they are good ideas, unless proven otherwise.
-
oh, so THAT'S what you were doing!
sorry, i didn't mean it in a BAD way, i just wasn't sure exactly what you were doing it or why you were doing it that way (did i mention OOP is a relatively new concept to me?)
-
Original XCom was programmed in procedural style with giant array of different global variables and arrays of structures.
So what's wrong with singleton? Is it out of oop paradigm? Or it's make mess in code?
-
Original XCom was programmed in procedural style with giant array of different global variables and arrays of structures.
So what's wrong with singleton? Is it out of oop paradigm? Or it's make mess in code?
A singleton is (not just IMO) a glorified global pointer. Which IS what I am proposing, and I could live with Game::instance()->foo() instead of theGame->foo().
-
Oh boy programming debates, these are always fun! ::) Eight posts in and there's already various opposing justifications and viewpoints without any lead programmers even posting.
To stay on topic I'm just gonna address the points about the project, also note this is all just my opinion and I can't speak for Daiky and if I had done it today I probably would've done it completely differently and etc etc.
- Why are we not using YAML anchors and aliases instead of thesaveId() functions? The file structure will be cleaner, and so will the code.
Don't YAML nodes just point to others? You'd still have to lookup the equivalent data structure in SavedGame, so I dunno about making the code clearer, although I guess the file format would be. But I don't know what yaml-cpp's support for them is like, there's no code examples.
- Instead of creating the objects and then loading them, why don't we have a "loading" constructor? Again, the code will be cleaner, since we can remove some functions that only get used once.
Consistency I guess. Not every object is only loaded on construction (eg. modular rulesets), and it leaves the objects more open for alternate loading later.
- Why don't we have the Game object as a global object (or Singleton)? It is required in LOTS of places and we just store and pass it around (or it's Ruleset and SavedGame members, which amounts to the same thing). Making it visible everywhere would simplify LOTS of code and remove redundant arguments from lots of functions. It's not like we will ever have more than one.
Singletons are... debatable. I don't really like them and I've always been taught not to like them. I don't see the need to arbitrarily restrict a class.
In this case you just want it as a global so you don't have to pass around pointers, but I don't think that really fixes the problem. Most game entities shouldn't need or even care about the entire game state, they just need particular bits. Game just started off as an abstraction for the core engine stuff but kinda grew into this big encompassing class because well, stuff has to go somewhere, and as particular elements needed particular stuff it just went all over the place. I guess optimally Game should be broken up so elements only need to reach what they need or something? I don't know how to properly redesign it so this wouldn't happen, OOP is great in theory until you find yourself not knowing how to actually put it into practice outside isolated examples, and you can end up going mad debating such design into infinity if you don't move on.
- Since we gave get/set for every member in some classes, why not make them plain structs?
Whether it's better to have public fields or getter/setters is arguable. Personally I just do it for the consistency, if I do something once I do it all the time, and it makes fields easier to control and document (specially since OpenXcom is ever-changing). Yeah it becomes a lot of boilerplate code in some classes, but there's plenty of tools to generate it for you.
- Why do we return pointers to collections (eg Game::getUfos()) instead of references? The code has a lot of places where references are more suited to what we mean instead of pointers (eg Everywhere we store Game, Rule* and SavedGame pointers in classes as fields).
Again, consistency. Probably has to do with old habits, I've always worked with pointers, so when references popped up I never got into them. What is their advantage over pointers? Aside from hiding the * which I don't really mind.
- Why do we return non-const rules? They should not have writable fields after they are initialized.
Aren't rules read-only already? Anyways const-correctness is one of those things that takes forever to implement and forever to master. I don't know everywhere where it should go, though I've done my best to apply it across OpenXcom without breaking anything, so feel free to add more if you want.
Classes having their data blindly manipulated from others directly is not OOP.
OOP means having objects represent ideas and keeping interactions to a minimum, each object containing all the data it needs and preneting the actions that make sense. So, for example, the Ufo should present methods for moving, damaging it or checking for interesting conditions (destroyed, landed, moving). Not give free access to everyone to directly set member variables. Moving all moving things on the map should be the responsibility of the class holding the list of all movables on the map, etc for many things.
There's always a new evil going around in programming circles. Back in my day, getters and setters were all the rage. :P
Seriously though, I've seen this argument go around a lot with OpenXcom. "Oh no, your code is just getters/setters instead of proper abstractions, the horror!". I'll admit I'm not an amazing coder and OpenXcom isn't the pinnacle of design. But have you tried implementing game rules, especially for strategy stuff like X-Com? You'll be shocked to know most of it is just getting/setting values, there's no big deep simulation or behavior or whatever, and trying to fully abstract it all away results in design nightmares because game rules aren't all nice and well-behaved, they're tons of values interacting with each other.
For example, take a Craft. You think "oh a craft has to move to places". So you need to change its position, just add a "setPosition". Then you think "but but but the craft should move to places, not be directly controlled". Ok you instead put a "setDestination" (maybe you'd prefer calling it "moveTo", it's just semantics), it does what it says, you give it a destination and the Craft slowly goes there by its rules. Woo, OOP, etc. But wait what about when you need to move the craft instantly, like when it's transferred between bases? Whoops turns out you needed that direct position control after all! And you'll run into this kind of stuff all the time, and your wonderful design goes to pot as you need to get stuff done and players just want to play things.
Our Game object now has 3 distinct roles. It's the main loop, the game state database and the rules database.
The main loop is a service and should be a global object.
Many things require the ruleset, and we only ever care for one, so a global ruleset is not unreasonable.
Since our objects don't have a clean state separation (data and behaviour are split across many objects), we need to pass the savegame around. But that is because it too has two roles. It's the 'persistent data' and also the 'current game state of all objects'. The 'current game state' is also a candidate for a global object, to be used everywhere except load/save routines (which actually care for the persistent data).
I'm not sure I understand your definitions here, so this is the way I see the OpenXcom design:
- The Game class handles the core engine stuff, the SDL subsystems, the state and event management. It also contains the game contents (resource, ruleset, save) out of convenience.
- The states take care of all the game behavior and interaction with the player, and are what connect and interact with everything else forming a running game.
- The SavedGame and related classes are intended to contain the variable game state, everything that needs to be saved to disk to preserve the game. The 'current game state of all objects' is the 'persistent data', so serializing Savegame saves the game and preserves the game state. This is ignoring superficial stuff which you might consider "state" like UI etc. Originally it was just intended to be "dumb" variable storage that the states would manipulate, but some behavior ended up leaking into it, so now it's kinda mixed between states and savegames.
- The Ruleset and related classes are intended to contain the fixed game variables. This is all the fixed definitions in the game rules, all the same fixed values from game to game. No behavior.
Hope that clears things up.
-
First of all, thank you for answering. I'll try to justify my positions and / or provide clarifications.
Don't YAML nodes just point to others? You'd still have to lookup the equivalent data structure in SavedGame, so I dunno about making the code clearer, although I guess the file format would be. But I don't know what yaml-cpp's support for them is like, there's no code examples.
I agree that this part of yaml-cpp is not clear. However, what it would allow us to do is to avoid the whole 'saveId()' part. We'll just have out << YAML::Anchor(toAnchorText(object));
object->save(out);
. . .
out << YAML::Alias(toAnchorText(other->ref_object));
instead ofobject->save(out);
. . .
other->ref_object->saveId();
True, it's longer, but I believe it's cleaner as we don't replicate YAML, we specificaly state what we do (emit a reference to an existing object). And we don't have to create the 'saveId()' part, it's as if the whole node is duplicated. It will also make it easier to abstract I/O away for object hierarchies.
Consistency I guess. Not every object is only loaded on construction (eg. modular rulesets), and it leaves the objects more open for alternate loading later.
I can understand the _more open_ part. But right now every class with I/O is either constructed and filled (via setters), or constructed and effectivelly overwritten (with 'load()'), except the ruleset, which is only partially overwritten in 'load()'. Which, I guess, is the reason for this API.
Singletons are... debatable. I don't really like them and I've always been taught not to like them. I don't see the need to arbitrarily restrict a class.
Singletons are, IMO, pretty-OOPified globals. But the idea of having globals is not bad in itself. In OpenXcom, we already use globals / singletons (RNG and Ufopaedia). The idea of a single GameEngine at least is not extreme.
In this case you just want it as a global so you don't have to pass around pointers, but I don't think that really fixes the problem. Most game entities shouldn't need or even care about the entire game state, they just need particular bits.
Most stuff needs the pointers only during I/O and creation time, but since eg Game is not global we have to keep it around for when we just need to pass it down to another class. And this tends to proliferate.
Whether it's better to have public fields or getter/setters is arguable. Personally I just do it for the consistency, if I do something once I do it all the time, and it makes fields easier to control and document (specially since OpenXcom is ever-changing). Yeah it becomes a lot of boilerplate code in some classes, but there's plenty of tools to generate it for you.
I agree, getters / setters are the best practice, since they allow you to change this internal representation. But due to the ad-hoc class APIs we are not reallly hiding anything. Still, keeping them allows us to eventually move to a better design.
Again, consistency. Probably has to do with old habits, I've always worked with pointers, so when references popped up I never got into them. What is their advantage over pointers? Aside from hiding the * which I don't really mind.
A reference states (in code) that what you want is always going to be there and will never change.
class Ufo
{
private:
const RuleUfo &_rules;
};
This means that a Ufo requires a rules object and it will be the same for the lifetime of the Ufo object.
They also allow the beautiful (IMO) getter/setter functions:
class Example
{
private:
int _field;
public:
int field() const { return _field; } https://Getter
int &field() { return _field; } https://Setter
};
Aren't rules read-only already? Anyways const-correctness is one of those things that takes forever to implement and forever to master. I don't know everywhere where it should go, though I've done my best to apply it across OpenXcom without breaking anything, so feel free to add more if you want.
Will do so, gradually, thanks for the ok.
For example, take a Craft. You think "oh a craft has to move to places". So you need to change its position, just add a "setPosition". Then you think "but but but the craft should move to places, not be directly controlled". Ok you instead put a "setDestination" (maybe you'd prefer calling it "moveTo", it's just semantics), it does what it says, you give it a destination and the Craft slowly goes there by its rules. Woo, OOP, etc. But wait what about when you need to move the craft instantly, like when it's transferred between bases? Whoops turns out you needed that direct position control after all! And you'll run into this kind of stuff all the time, and your wonderful design goes to pot as you need to get stuff done and players just want to play things.
I understand what you mean, but the specific example IS flawed. A Craft does not "move" (fly in Geoscape) between bases, and the owning base is a matter of game database. So the destination has nothing to do with it. You need a 'setBase()' (which, in an ideal world, only transfers would use).
I'm not sure I understand your definitions here, so this is the way I see the OpenXcom design:
- The Game class handles the core engine stuff, the SDL subsystems, the state and event management. It also contains the game contents (resource, ruleset, save) out of convenience.
- The states take care of all the game behavior and interaction with the player, and are what connect and interact with everything else forming a running game.
- The SavedGame and related classes are intended to contain the variable game state, everything that needs to be saved to disk to preserve the game. The 'current game state of all objects' is the 'persistent data', so serializing Savegame saves the game and preserves the game state. This is ignoring superficial stuff which you might consider "state" like UI etc. Originally it was just intended to be "dumb" variable storage that the states would manipulate, but some behavior ended up leaking into it, so now it's kinda mixed between states and savegames.
- The Ruleset and related classes are intended to contain the fixed game variables. This is all the fixed definitions in the game rules, all the same fixed values from game to game. No behavior.
Hope that clears things up.
Yes, things are cleaner now. What I meant was that the 'keeper of rules' and 'keeper of persistent data' should be outside Game, in top level objects of their own. And that would make these three classes good candidates for singleton / global data. Since what they do is provide a service to all the rest of the code, so instead of having a pointer to them in each class, we just keep a global pointer of them. Like we (effectivelly) do with RNG and Ufopaedia.
-
The YAML format for save games is very explicit about default values. Its quite inefficient, space wise, and slow to load on large saved games (e.g. enlarged battles)
The bulk of an enlarged battle state is tile state. Most of the tiles are not on fire. They have no smoke. They are (not yet) discovered. These facts are all explicitly stored in YAML.
They can be defaulted in the load code, and the save code can emit YAML only if the content is not an implicit default.
Omitting fire: 0 and smoke: 0 in tile saves, and then discovered[false,false,false] reduced file sizes (in KB) as shown. (It may reduce save/load times)
george-russells-imac:OpenXcom grrussel$ ls -sk boom3.sav
4624 boom3.sav
george-russells-imac:OpenXcom grrussel$ ls -sk test_sav_change.sav
3928 test_sav_change.sav
george-russells-imac:OpenXcom grrussel$ ls -sk test_sav_change2.sav
3164 test_sav_change2.sav
The tile positions are also explicitly given, and could likewise be implicit due to ordering in the sequence.
-
@grrussel:
The man who wrote the save/load code; i also didn't understand why he made everything explicit.
Beyond what you have described (which you have totally right about) the backwards compatibility is also worse this way!
If there were default values, then problems like this could be avoided:
"hyperDetected:" is introduced, and now we have to convert all savegames manually by adding "hyperDetected: false" for every ufo.
And every single addition like this produces this problem, which could be avoided with default values.
So i also vote that the save/load code should be changed to work with default values.
-
while i agree with you i also feel it is worth stating:
backwards compatibility of saved games should not be a concern in what is essentially a development version.
Post 1.0, yes, this should DEFINITELY be taken into account, but at this stage of development, new versions breaking old saved games is par for the course.
i say just be thankful that it is saved in a plain-text format that allows you to make old saved games compatible with new revisions.
-
while i agree with you i also feel it is worth stating:
backwards compatibility of saved games should not be a concern in what is essentially a development version.
Post 1.0, yes, this should DEFINITELY be taken into account, but at this stage of development, new versions breaking old saved games is par for the course.
I'm 100% with warboy on this. Although saving only deviations from the default for the tiles (and even a little less verbose format for tile data) should be possible and desirable.
-
while i agree with you i also feel it is worth stating:
backwards compatibility of saved games should not be a concern in what is essentially a development version.
Post 1.0, yes, this should DEFINITELY be taken into account, but at this stage of development, new versions breaking old saved games is par for the course.
i say just be thankful that it is saved in a plain-text format that allows you to make old saved games compatible with new revisions.
I think that's the whole point of this problem. After 1.0 the savegame format will not change. (...much, or so frequently)
So after 1.0 the backward-compatibility doesn't needed anymore. (everyone convert his savegame when 1.0 comes out, and that's it)
-
you make a good point. i think the best compromise here would be some sort of "savegame updater" tool, and i have a friend making one as i type this.
-
The default-valued save/load handling is a better version, you already agreed that.
Now. What would happen if i change the whole save/load handling on my own, and make it in a PR?
Will you - and everyone - then follow that default-value convention for the newer features?
If yes, then i'll make this change.
-
i can't speak for everyone, but if it's something as simple as adding an if statement then yeah, i see no issue.
i don't really understand the YAMLCPP library at all, or how the save code really even works. i just know that it does, and that's enough for me :P
-
Ok then:
@SupSuper: If i make this, will you prescribe the default-valued save/load convention as a coding guideline?
-
Ain't hindsight a wonderful thing? :P
You gotta realize, OpenXcom is kinda old. We've been at this for over 3 years now, it starts to show. Much like any coder will look at anyone else's code and go "pfffff this is terrible, it should be done this way", they will also have the same reaction just looking at their own code after a few months. I was still starting university when I began OpenXcom and now that I'm almost finishing I see code completely differently. Different code was written at different times with different knowledge and different priorities (and since it's OSS, often by different people), so eventually it all starts to look a bit schizophrenic.
So about the load/save code. It wasn't implemented from the start, it was added in later in 0.2. And as anyone knows, serializing in C++ is a colossal effort, specially if you don't do it from the start, because there's no convenient reflection or whatever, you have to go through every single class and add in the appropriate code manually, and debug throughly all the mistakes you're bound to make. It took forever, it was a huge pain, but in the end it worked and that was all we cared about. No real concern for performance or maintainability or whatever, that would all just be even more extra effort and code, and our priorities weren't in it.
So of course there's a bunch of things wrong with it. I bet I know more of them than you. Yes, it's not size-efficient, it just dumps all the data straight into files, not accounting for default values and always breaking when something is added. It's also slow, although this is mostly yaml-cpp's fault, I've discussed it with their developer. For example, did you know accessing a keyvalue in a map node (eg. node["value"]) is actually a O(N) operation and not a O(1) operation as you'd expect? yaml-cpp manually looks up the value in the map everytime instead of keeping an index or whatever. The workaround is to iterate through the keyvalues yourself, instead of doing lookup. Even then, yaml-cpp is still slow (libyaml has it completely beat, it loads even our massive saves in a few secs, but it's C-based). Not only that, the current API we're using for yaml-cpp is deprecated, so the whole thing probably needs to be rewritten anyways. Yeah. That sounds like a fun time, deciding all that stuff. :P So it's been in the backburner for a while.
For comparison, the ruleset loading code, which came later, accounts for default values and the O(N) problem, so if you want a better example of yaml-cpp code to follow, that would be it. Of course this makes it much more verbose, basically consisting of huge else-ifs. And given all the underlying issues anyways, you really gotta consider if it's worth just keep on "patching" the existing load/save code.
@grrussel:
The man who wrote the save/load code; i also didn't understand why he made everything explicit.
Beyond what you have described (which you have totally right about) the backwards compatibility is also worse this way!
If there were default values, then problems like this could be avoided:
"hyperDetected:" is introduced, and now we have to convert all savegames manually by adding "hyperDetected: false" for every ufo.
And every single addition like this produces this problem, which could be avoided with default values.
So i also vote that the save/load code should be changed to work with default values.
OpenXcom pre-release is not supposed to be backwards-compatible. At all. I want the freedom to make and break the save structure whenever I want. When the game is nice and stable and pretty, sure, backwards-compatible for everyone, but not now. How are you gonna deal when the alien AI gets added and people already have saves in 3000 with everything unlocked and none of the required alien infrastructure?
Not only that, you're not supposed to keep carrying saves from old pre-release versions. You're not supposed to be trying to play for completion here, but testing every version brand new. If it's too heart-breaking to always start fresh whenever something changes, tough, that's testing for you, it's mindlessly repetitive. Old versions might carry old bugs that bleed into the saves, and constantly carrying and patching them around might just end up producing non-bugs later down the line. Testing is useless if you can't be sure the origin is actually the thing you're testing.
I think that's the whole point of this problem. After 1.0 the savegame format will not change. (...much, or so frequently)
So after 1.0 the backward-compatibility doesn't needed anymore. (everyone convert his savegame when 1.0 comes out, and that's it)
This is an extremely optimistic view. If you really expect OpenXcom to just freeze on 1.0 and don't see how much stuff keeps getting added because people keep wanting more, you've got another thing coming. The version number in the save isn't just for show, it's a precaution.
-
re the version number in the save file; its probably wise to bump it immediately after a release - as now we will have 0.4.5 tagged save files with various features ;-)
This looks currently set from Options.cpp
std::string _version = "0.4.5";
If you think the yaml-cpp is slow, you should see the load time of the save game files in python!
FWIW: the speed of save load/store is not noticeable so far for normal XCOM - only with my way larger battlescape sizes.
-
I want the freedom to make and break the save structure whenever I want.
How the default-valuing breaks this freedom? I think you could break the structure at any time with default-values too.
How are you gonna deal when the alien AI gets added and people already have saves in 3000 with everything unlocked and none of the required alien infrastructure?
I don't see the problem here. It would have its default value. I admit that those games might look a little schizophrenic because some parts look like 'the game just begun' after game-loading.
And actually i don't expect FULL backwards-compatibility from the default-valuing, the main purpose to handle the MINOR changes, like adding things like "hyperDetected". And the second main purpose is the size and speed efficiency, of course.
The version number in the save isn't just for show, it's a precaution.
Yes, i know that.
But still, if someone wants to migrate a savegame to a new version, he would have a better chance this way, by only setting the version number in the savegame.
-
The proper fix for default values is to have ALL fields initialized in a class' constructor with values representing the default, and perform conditional overwrite in [oad(). For some fields it may not really work, but it's the best possible solution.
Still, savegame compatibility right now IS a useless goal. The game is still in flux, saving a game should only be considered part of bug hunting (keep a save to have a test case for reproducing a bugs).
-
The proper fix for default values is to have ALL fields initialized in a class' constructor with values representing the default, and perform conditional overwrite in [oad(). For some fields it may not really work, but it's the best possible solution.
That's what i thought.
And a conditional save() is also needed, checking if the value to be written is equal to the default or not. (and only write out if it's not the default)
-
Regarding yaml-cpp deficiency:
Using the if-else and iterating in order is not significally faster, we still do O(N) comparisons for each node.
In addition, the code is harder to read and we'll get no benefit if/when yaml-cpp changes to keep values in maps instead of vectors. Using code like the following is my best solution so far:
const YAML::Node &node = doc["thingy"];
node["field1"] >> variable;
node["field2"] >> variable;
The code above only searches for "thingy" once, and the follonode["field1"] >> variable;wing searches are (hopefully) in the smaller contents.
And it is still easier to read and understand.
-
That's what i thought.
And a conditional save() is also needed, checking if the value to be written is equal to the default or not. (and only write out if it's not the default)
For smaller save games yes. But not for compatibility. I don't think the savings will be enough to go to the trouble of doing it for every little field. If the size becomes a problem, it's much better to simply keep the files compressed.
-
Don't forget if we have smaller savegames, then the O(N) means much less. :)
And this is the point for having smaller savegames, compressing it is not reducing the O(N).
-
Don't forget if we have smaller savegames, then the O(N) means much less. :)
But if you remove fields, you are always searching the entire list for nothing! ;D
-
But the entire list is a LOT smaller. :)
And BTW, if you read it without checking isn't that searching the entire list also?
-
My post was about the load/save code in general, not default-values. If you wanna implement default-values everywhere be my guest, I agree it's a good idea, just be aware that it might be a fruitless effort since the load/save code might just be rewritten later, and it will be a massive amount of work as the load/save code is huge and probably very bug-prone (not everything has immediately obvious default-values), so you better know what you're doing.
re the version number in the save file; its probably wise to bump it immediately after a release - as now we will have 0.4.5 tagged save files with various features ;-)
This looks currently set from Options.cpp
std::string _version = "0.4.5";
I use the version number more to tempt people when a new version is coming. ;)
Using the if-else and iterating in order is not significally faster, we still do O(N) comparisons for each node.
Doing N iterations is a lot cheaper than N comparisons. :P But yes I clearly stated that this isn't a huge deal, since the N amount of elements in a savegame yaml map is in the dozens at most.
Using code like the following is my best solution so far:
const YAML::Node &node = doc["thingy"];
node["field1"] >> variable;
node["field2"] >> variable;
The code above only searches for "thingy" once, and the follonode["field1"] >> variable;wing searches are (hopefully) in the smaller contents.
And it is still easier to read and understand.
So leave it as is? :P
The proper fix for default values is to have ALL fields initialized in a class' constructor with values representing the default, and perform conditional overwrite in [oad(). For some fields it may not really work, but it's the best possible solution.
This is also how ruleset loading already works.
-
Doing N iterations is a lot cheaper than N comparisons. :P But yes I clearly stated that this isn't a huge deal, since the N amount of elements in a savegame yaml map is in the dozens at most.
When we just use node[val] for each of our values we do values searchs, each search is O(nodes). So we do O(N) * O(N) == O(N^2) actions.
When we iterate we do 'nodes' iterations * value comparisons, so still O(N) * O(N) == O(N^2) actions.
So they are exactly the same, but iteration is still the same cost if/when yaml-cpp changes to maps.
Just increasing my post count! ;)
-
the load/save code might just be rewritten later,
How much later? Approximately.
-
How much later? Approximately.
By 1.0? Whenever the old yaml-cpp API is gone for good? I dunno I'm not exactly looking forward to it either but I wouldn't stress over it, just don't get too attached. :P
-
If you were about to rewrite, how would you rewrite it?
You mentioned libYAML, maybe we can change to that.
It's C - as you also mentioned - but i think the compilers can handle it.
Besides, github tells me we already have 3% C code. :)
-
If you were about to rewrite, how would you rewrite it?
You mentioned libYAML, maybe we can change to that.
It's C - as you also mentioned - but i think the compilers can handle it.
Besides, github tells me we already have 3% C code. :)
The problem isn't that it's C per-se (after all, SDL is C-based). It'll just be a lot uglier since you won't get all the niceties from C++, like STL (vectors etc.), operator overloading, classes, etc. This is why a lot of OpenXcom code is designed to just wrap SDL's ugly C-style calls, making them more OOP-friendly.
Look, I could be speculating and giving advice all day. If you really wanna experiment with it, nothing like getting your hands dirty. Try whatever you want, try the default-values, try different libraries, try different methods, etc. Only then will you actually get to experience what works and what doesn't, and even if it doesn't work out, you'll have learned something from it.
-
i've messaged a few people about this, but i may as well post it here too.
i'm not sure engine-state-extensions was a good idea, it's introduced far more problems than it's solved (i don't think there WERE problems? if it wasn't broken, why did we fix this?)
a lot of the logic now needs to be processed in reverse order, and it breaks a lot of things.
ufo detection popups, for example, are missing a _pause = true; so when playing at speed = 1 day, when you click center on ufo, it may be on the other side of the planet, or have left it already.
this is an easy fix, but look at research now, it's a mess. it's all in the wrong order, and correcting it would be a major undertaking. i tried reversing the order in which the states are added, and although this fixes the ORDER, i can still see the screens behind it (newPossibleResearch) because it is larger. other things are happening in the wrong order/overlapping eachother as well, for example, at the end of the month, i got a report screen, but there was a terror site detected screen drawn on top of it.
and in the time5Seconds routine, the craft logic needs to happen BEFORE the ufo logic, i'm sure there's a lot more that needs to be re-ordered to accomodate this.
no offense to karvanit, but i think this was a bad pull, i'm sure it's not unfixable, but it seems more effort than necessary when we could simply revert the commit and fix it immediately and with no effort.
unless there's a good reason for the change, in which case, please disregard.
-
The problem isn't that it's C per-se (after all, SDL is C-based). It'll just be a lot uglier since you won't get all the niceties from C++, like STL (vectors etc.), operator overloading, classes, etc. This is why a lot of OpenXcom code is designed to just wrap SDL's ugly C-style calls, making them more OOP-friendly.
Well, we could do the same for libYAML, making 1 or 2 new files which makes libYAML OOP-friendly. That's part of introducing it. :)
Look, I could be speculating and giving advice all day. If you really wanna experiment with it, nothing like getting your hands dirty. Try whatever you want, try the default-values, try different libraries, try different methods, etc.
Yeah, i just want to avoid solutions which you may reject at the first place. (before making it)
This is a so much major thing about the project, i think.
Only then will you actually get to experience what works and what doesn't, and even if it doesn't work out, you'll have learned something from it.
Well, if something i learned in my time on the university, - which i'm just finishing just like you :) - is that planning is NEVER enough! :)
More planning -> less problems later. It's almost every time true.
EDIT:
I have an idea.
Do you remember the DLL-hell / Dependency nightmare?
What would happen if we integrate all the external libraries?
I mean we remove the deps folder, and take all the source files from the external libraries and put them to (eg.) src/ext_libs ? (they would be actually stored on github!)
I think we can avoid the dependecy nightmare, and we always have STATIC linking easily, without the need of any more configuring.
And of course noone needs to get the external libs when starting to coding. If a new version of an ext.lib. comes, then its just updated with a github-commit.
-
Re: The engine-state-extension fiasco: :-[ :'(
Re: Pulling in all external libraries:
I am deeply opposed but that may be because working in Linux getting them is easy.
Re: libyaml
It appears to be extremely low level. One would essentially rewrite YAML-cpp if one tries to make it OOP, or at least that's what I can see from the API in the wiki.
-
Re: Pulling in all external libraries:
I am deeply opposed but that may be because working in Linux getting them is easy.
Would anything be harder for you? (after all you wouldn't have to get it at all)
-
No, it just bloats the repository. :D
The proper (tm) way to do this would be to have seperate repositories for each library and use git submodules. Which are not familiar to many people.
-
No, it just bloats the repository. :D
And why is this a problem? :)
The proper (tm) way to do this would be to have seperate repositories for each library and use git submodules. Which are not familiar to many people.
Maybe, but it's easier to have a new folder with the files in it.
-
Speaking about linux and yaml.
Is yaml-cpp is better choice than libyaml?
Since libyaml is present in the repository of any decent linux distribution. So building and packaging the game will have less hassle.
-
Speaking about linux and yaml.
Is yaml-cpp is better choice than libyaml?
Since libyaml is present in the repository of any decent linux distribution. So building and packaging the game will have less hassle.
In Ubuntu yaml-cpp IS in the distribution, and I think it's also in Debian. Don't know about the others.
But: Looking at libyaml again, it appears it provides a high-enough level that we could wrap it in a few classes and use it. Perhaps it'll be worth it just after we have all the functionality in the game.
Programming rule: Make it compile => Make it run => make it run correctly => make it run fast
-
In Ubuntu yaml-cpp IS in the distribution, and I think it's also in Debian. Don't know about the others.
Not quite.
In Ubuntu the yaml-cpp (https://launchpad.net/ubuntu/+source/yaml-cpp/0.3.0-1) package first released '2012-06-08' and it's available ONLY to Quantal and forth. The last LTS Precise Pangolin lacks that package so I have to made one for myself.
In Debian libyaml-cpp0.3 (https://pkgs.org/debian-wheezy/debian-main-i386/libyaml-cpp0.3_0.3.0-1_i386.deb.html) package first released '2012-06-03' for Wheezy.
So (strictly speaking) yaml-cpp library was adopted by Linux distributions a couple of months ago and considered experimental.
IMO it's better to base on technology that distributed and supported better.
-
I have an idea.
Do you remember the DLL-hell / Dependency nightmare?
What would happen if we integrate all the external libraries?
I mean we remove the deps folder, and take all the source files from the external libraries and put them to (eg.) src/ext_libs ? (they would be actually stored on github!)
I think we can avoid the dependecy nightmare, and we always have STATIC linking easily, without the need of any more configuring.
And of course noone needs to get the external libs when starting to coding. If a new version of an ext.lib. comes, then its just updated with a github-commit.
You try that yourself and let me know how it goes. You can never get rid of dependency hell, it's a necessary evil. I've been in a lot of projects with a lot of different approaches to the problem, like pre-built files, custom own tools to automatically get dependencies, hooking them into the repository through externals/submodules, or just not giving a shit. :P In the end it's always a hassle.
The problem with static linking is it isn't a magic wand, or everyone would do it, you're just shifting the workload to the developers. Now the user doesn't need to worry about DLLs since it's packed into the EXE, but the developer needs to build every dependency themselves, including dependency's dependencies, and dependency's dependencies' dependencies, and... you're kinda undermining the advantage of using someone else's work. Also some libraries have licenses that restricts static/dynamic linking, or actually make use of dynamic linking at runtime (eg. SDL_mixer only loads smpeg if a format needs it).
As for "copying" them to your repository, again you have to maintain and build everything yourself, make sure it stays up to date, pollute your commits with third-party updates, check everything builds in the right order, etc. Also you usually can't just "copy" other people's work into your own, you need to get permission, give credit, check if the licenses are compatible, etc. Another solution is having your repository linked to other repositories (through externals/submodules), so you're always up-to-date, but then you're relying on everyone's bleeding-edge code from their repositories, and we already have enough bugs around here as it is. :P