State of ValueRefs Implementation

Programmers discuss here anything related to FreeOrion programming. Primarily for the developers to discuss.

Moderator: Committer

Post Reply
Message
Author
Ophiuchus
Programmer
Posts: 3427
Joined: Tue Sep 30, 2014 10:01 am
Location: Wall IV

State of ValueRefs Implementation

#1 Post by Ophiuchus »

I am working on registering ValueRefs under a name and I found some stuff which got in my way. This blocks parts of the current implementation which i will probably mostly ditch, but i still would like to get some insight and move the parser/valuerefs in a better state.
  • some valuerefs get parsed multiple times (e.g. fuel tanks are parsed four times, reinforced hull tech gets parsed only once) - is this by design or wrong?
  • ValueRefs are not copyable - is this by design? I would like to make a copy of a valueref mid-parsing and the only solution i can imagine working is adding copy constructors for basically all valuerefs. (or some other variant of polymorphic cloning). Would it be acceptable to add explicit copy constructors?
  • usage of unique_ptr and the movableenvelope. Probably this was introduced to prevent memor i yleaks but it really increases maintenance complexity in the parsers by at least one or two orders of magnitude. Does anybody have details? Would shared_ptrs be acceptable?
  • having discrete valuerefs for integer and double in all cases looks like it makes the whole thing also more complex. would using variant type make more sense?
I am drawing on your knowledge/opinions here, please offer your wisdom, gods of freeorion cpp :lol:
Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

Look, ma... four combat bouts!

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13586
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: State of ValueRefs Implementation

#2 Post by Geoff the Medio »

Ophiuchus wrote: Tue Jun 23, 2020 2:01 pmsome valuerefs get parsed multiple times (e.g. fuel tanks are parsed four times, reinforced hull tech gets parsed only once) - is this by design or wrong?
What is a "valueref" in this context? There are no lists of ValueRefs that just get parsed... they're all attached to content like techs or ship parts, which are all parsed independently, presumably once each per process (server, human client, each AI client).
ValueRefs are not copyable - is this by design?
The current design is that effect-condition-valueref trees have a simple parent-child relationship, with each element having a single parent, with its lifetime bound to the parent's lifetime. If there are two bits of content that have the same Condition or ValueRef in them, then there are two copies of that same Condition or ValueRef. These aren't very big data structures, so this isn't a problem, as far as I know. Also, there are some use cases where having a separate copy per top-level content object is useful, such as some ValueRefs that can return the name of that top level content for use in scripts. If there were multiple owners of the same ValueRef, that would get trickier.
I would like to make a copy of a valueref mid-parsing and the only solution i can imagine working is adding copy constructors for basically all valuerefs. (or some other variant of polymorphic cloning). Would it be acceptable to add explicit copy constructors?
Possibly, but I doubt it's necessary. For your application, I would probably have a special "Named" ComplexVariable, that takes a single string parameter. That string would be looked up in a completely separate list of ValueRefs, stored by name, and the result of evaluating the "Named" ComplexVariable ValueRef would be the result of evaluating the ValueRef from the list.
usage of unique_ptr and the movableenvelope. Probably this was introduced to prevent memor i yleaks but it really increases maintenance complexity in the parsers by at least one or two orders of magnitude. Does anybody have details? Would shared_ptrs be acceptable?
I think the motivation for the unique_ptr and MovableEnvelope was memory management, and to conform to (a particular?) C++ standard practice where unique things that should have only a single manager / container managing their lifetime are stored in unique_ptr, which is applicable for all the parsed content classes. shared_ptr would be more suitable if multiple places needed to have lifetime-managing pointers to the objects, which isn't applicable in these cases.

I don't fully understand what the MovableEnvelope wrapper is doing or how, but I haven't needed to. It's not really impacted my modifications of the parsers. Your particular application is somewhat more affected by it, but it sounds like you're trying to do things contrary to how the whole system is designed and implemented, which ended up getting more complicated to get working.
having discrete valuerefs for integer and double in all cases looks like it makes the whole thing also more complex. would using variant type make more sense?
It's not clear what you mean, but I don't think it would make sense. I recently added an untyped ValueRefBase in master, which adrianbroher doesn't like much, but which was useful for other things. But for the ValueRef<T> classes, they all share a similar interface but do different things due to the different stuff that they can be used for. When parsing a ValueRef<std::string> or a ValueRef<int>, different expressions are valid. And when evaluating one, different code is used to decide what to return. I don't really see the advantage of switching to storing a boost::variant... You'd need to specify what type is expected as output when evaluating, instead of when defining the ValueRef, I suppose? How would that be helpful?

Ophiuchus
Programmer
Posts: 3427
Joined: Tue Sep 30, 2014 10:01 am
Location: Wall IV

Re: State of ValueRefs Implementation

#3 Post by Ophiuchus »

Geoff the Medio wrote: Tue Jun 23, 2020 8:13 pm
Ophiuchus wrote: Tue Jun 23, 2020 2:01 pmsome valuerefs get parsed multiple times (e.g. fuel tanks are parsed four times, reinforced hull tech gets parsed only once) - is this by design or wrong?
What is a "valueref" in this context? There are no lists of ValueRefs that just get parsed... they're all attached to content like techs or ship parts, which are all parsed independently, presumably once each per process (server, human client, each AI client).
I do not think this happens once per process. If I register a value ref while parsing and send that to the logger, i see four entries for fuel tanks in the client log and four entries in the server log (did not check for AI yet).

And as i said the number of the parses is different for different content. I get a single entry for the tech and four entries for the ship part.
Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

Look, ma... four combat bouts!

Ophiuchus
Programmer
Posts: 3427
Joined: Tue Sep 30, 2014 10:01 am
Location: Wall IV

Re: State of ValueRefs Implementation

#4 Post by Ophiuchus »

Geoff the Medio wrote: Tue Jun 23, 2020 8:13 pm
ValueRefs are not copyable - is this by design?
The current design is that effect-condition-valueref trees have a simple parent-child relationship, with each element having a single parent, with its lifetime bound to the parent's lifetime. If there are two bits of content that have the same Condition or ValueRef in them, then there are two copies of that same Condition or ValueRef. These aren't very big data structures, so this isn't a problem, as far as I know. Also, there are some use cases where having a separate copy per top-level content object is useful, such as some ValueRefs that can return the name of that top level content for use in scripts. If there were multiple owners of the same ValueRef, that would get trickier.
I am not aware of ValueRefs can return the name of that top level content - do you have an example?

You are concerned of multiple owners, but I want a (deep) copy, so there would still be single owners involved. (Were you thinking of shared_ptr here?)

I read this as there is currently no need to create copies but there is not really a reason against copies?
Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

Look, ma... four combat bouts!

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13586
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: State of ValueRefs Implementation

#5 Post by Geoff the Medio »

Ophiuchus wrote: Wed Jun 24, 2020 8:27 amI am not aware of ValueRefs can return the name of that top level content - do you have an example?
Not sure exactly what you want to see, but: https://github.com/freeorion/freeorion/ ... s.cpp#L445
You are concerned of multiple owners, but I want a (deep) copy, so there would still be single owners involved. (Were you thinking of shared_ptr here?)

I read this as there is currently no need to create copies but there is not really a reason against copies?
You can add a bunch of Clone methods to ValueRef and Condition and implement it so that it returns a deep copy with no pointers to the original copied objects, while still storing all contained ValueRef and Condition with unique_ptr.

Ophiuchus
Programmer
Posts: 3427
Joined: Tue Sep 30, 2014 10:01 am
Location: Wall IV

Re: State of ValueRefs Implementation

#6 Post by Ophiuchus »

Geoff the Medio wrote: Wed Jun 24, 2020 5:58 pm
Ophiuchus wrote: Wed Jun 24, 2020 8:27 amI am not aware of ValueRefs can return the name of that top level content - do you have an example?
Not sure exactly what you want to see, but: https://github.com/freeorion/freeorion/ ... s.cpp#L445
Oh, of course. I do not fully grasp its use though. That means each node in the valueref tree gets this set top down, correctly?
And this is set once per value ref as opposed to being set dynamically before evaluation?

That means a named valueref needs to problably to be invariant for top level content(?). What should I use as top level content for a registered value ref if i "top level" register it?

Else this structure cant be used in contexts with different top level content. For the moment I just need to name/reference very invariant ValueRefs, but would it make sense handle.
Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

Look, ma... four combat bouts!

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13586
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: State of ValueRefs Implementation

#7 Post by Geoff the Medio »

Ophiuchus wrote: Fri Jun 26, 2020 10:02 ameach node in the valueref tree gets this set top down, correctly? And this is set once per value ref as opposed to being set dynamically before evaluation?
Yes. Each valueref or condition or effect has a SetTopLevelContent function that is called when its containing content is initialized. That calls then calls the SetTopLevelContent of its children, if it has any, or if it's ValueRef::Constant it sets its internal m_top_level_content. If it is a ValueRef::Constant<std::string> with its stored value set to "CurrentContent", it will return the top level content instead of the stored value.
That means a named valueref needs to problably to be invariant for top level content(?). What should I use as top level content for a registered value ref if i "top level" register it?
If it's a "free" valueref, then it shouldn't reference "CurrentContent" in any ValueRef::Constant within the tree, and nothing needs to be set. You don't need to call SetTopLevelContent for valuerefs, conditions, or effects that aren't part of a top-level bit of scripted content like a species or building type, as there's no top level content to set... but they also can't be written as if they were contained by some top-level content...

Although, I suppose you could make some named value refs in a container, and make sure they only get used for a single type of top level content. Then in SetTopLevelContent for the valueref that looks up another valueref by name, you could call SetTopLevelContent to pass along the top level content name to the named valueref. Maybe add a test in Constant<T>::SetTopLevelContent to log an error if the top level content is set twice, to make sure the named valueref doesn't get used in two places with different top-level content...? I suspect it would be easier and more useful to just prohibit using "CurrentContent" in FOCS expressions that are stored in named valuerfs, though, so that they can be used anywhere.

Ophiuchus
Programmer
Posts: 3427
Joined: Tue Sep 30, 2014 10:01 am
Location: Wall IV

Re: State of ValueRefs Implementation

#8 Post by Ophiuchus »

Geoff the Medio wrote: Fri Jun 26, 2020 11:32 amI suspect it would be easier and more useful to just prohibit using "CurrentContent" in FOCS expressions that are stored in named valuerfs, though, so that they can be used anywhere.
Agreed, but how could i enforce that more easily?

I could complicate parsing - somehow doing different things when in in a named valueref. One option mostly duplicating valueref parsers and leaving out current content or passing a flag and failing the parse.

I could also complicate evaluation I guess. Somehow passing a flag and failing on evaluation.

Or I would need to run a visitor along the value ref tree and analyse it - dunno how one does that in cpp.

I probably could add another flag for valuerefs like isFreeVariable() and fail on parsing if this gets passed as a named valueref.
Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

Look, ma... four combat bouts!

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13586
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: State of ValueRefs Implementation

#9 Post by Geoff the Medio »

I suppose adding another invariance flag or getter like these would be reasonable... named something like TopLevelContentIndependent() perhaps. That would return true if the ValueRef tree has no references to "CurrentContent" in it, meaning no ValueRef::Constant<std::string> with m_value == "CurrentContent".

If that is true, then the top level ValueRef would be safe to use inside FOCS within any content or as a free ValueRef that's not associated with any bit of scripted content.

I think it doesn't need to be a stored flag since this would only be evaluated as a warning or error while parsing scripting, and not when the valueref is evaluated repeatedly during gameplay.

Post Reply