Per-object machines, minimally functional fuel consumption #61

z-adams · 2021-01-24T09:05:29Z

This one makes up for the last PR, now you see why I split it into two :)

At a high level, the changes primarily add the ability for machines to be owned by non-part objects. The new PrototypeObject::m_machineIndices array allows a PrototypeObject to keep track of the machines it owns. When a PrototypeObject is created in AssetImporter::proto_add_obj_recurse(), its machines are loaded by AssetImporter::load_machines(). This function takes a reference to the ProtoObject's index array, and the parent PrototypePart's machine array. PrototypeMachines are added to the PrototypePart's master list, and the indices of the machines populate the PrototypeObject's index array. When a part is instantiated in SysVehicle::activate_sat(), the SysVehicle::part_instantiate() function creates an array of objects which track the relationships between the new object ActiveEnts and the PrototypePart's machine array, since the PrototypePart and PrototypeObject information is discarded once the entities have been loaded into the scene. A new function SysVehicle::part_instantiate_machines() takes this array, and uses it to add the appropriate machines to the various entities in the scene. This deferred action is necessary because machines may depend on the entire part's subhierarchy existing; the example given in the code is how MachineRocket needs the plume effect node to exist when it's instantiated.

The rest of the stuff is just a quick way of adding functional fuel consumption to MachineRocket. It's handled by an ugly and hardcoded solution right now (hardcoded tank ID, storing tank entity ID as MachineRocket member). This will change in the next PR, which generalized everything with a new "Pipe" wire type. If it's unacceptable to do the hardcoding (it currently renders the "moon" scene vessels immobile) I can include the next few commits in this PR as well, but it's already a large amount of code and making the PR 70% larger is going to suck.

The current functionality is basically that the 10m^3 fuselage tank is used to store a generic "fuel" which is drained by the rocket engines. When the fuel is depleted the engines cease to provide thrust, although the plumes keep working. The next PR will smooth out this proof of concept into a generalized system.

z-adams · 2021-01-24T09:06:49Z

src/osp/Resource/AssetImporter.cpp

+    if (node.extras.Has("machines"))
+    {
+        load_machines(node.extras, part.get_machines(), obj.m_machineIndices);
+    }


When each object is loaded, its machine configs are loaded here

z-adams · 2021-01-24T09:07:46Z

src/osp/Resource/AssetImporter.cpp

+        machineIndexArray.push_back(machineArray.size());
+        machineArray.emplace_back(std::move(machine));


PrototypeMachines are added to the PrototypePart's machine array, and the indices are stored in the PrototypeObject's machineIndexArray

Make sure to reserve space in the vector before calling emplace in a loop.

Note though that if this function is being called in a loop, don't reserve a whole bunch, instead figure out a way to get the total size the vect needs to be, and reserve once at the top of the stack.

For the machineIndexArray, that's easy (and I may actually just change that parameter to a return value). For the machineArray, it's not so easy. There's not really any way to know how many machines will be in the part's hierarchy unless we add a second recursion pass just to pre-allocate machine storage. I think it's probably best to just let std::vector do its thing and then maybe shrink it to fit afterwards, unless you think the allocations are really such a big deal that it's worth doing a pre-pass to figure out how much space it needs. Otherwise, I'll just leave a comment on line 149 for now that pre-allocation would be nice, and if we ever find ourselves needing to do multiple passes over the config for some reason, we can add this at that point.

src/osp/Active/SysVehicle.cpp

z-adams · 2021-01-24T09:09:01Z

src/osp/Active/SysVehicle.cpp

+        /* Deferred machine instantiation
+        * 
+        * Since machines may depend on the existence of other objects in the
+        * part's hierarchy, machine instantiation must be deferred until the
+        * entire hierarchy exists. For instance, a rocket engine's root
+        * "MachineRocket" machine may depend on the existence of a child node
+        * to receive an "ACompExhaustPlume" component that handles graphical
+        * effects related to the rocket machine, but that child node would not
+        * exist in time for the MachineRocket to be instantiated alongside
+        * the root part entity.
+        */
+        part_instantiate_machines(partEntity, machineMapping, proto, partBp);


After the part and objects have all been instantiated, the machines are instantiated

This sounds good. I recently de-spaghettified SysVehicle in my own branch, making lots of functions static like in my other PRs. There will be merge conflict for sure soon.

z-adams · 2021-01-24T09:17:47Z

src/osp/Active/SysVehicle.h

+    ActiveEnt part_instantiate(PrototypePart& part, BlueprintPart& blueprint,
+        ActiveEnt rootParent, std::vector<MachineDef>& machineDefinitions);


I meant to have this return a tuple and use structured bindings instead of the machineDefinitions output parameter; I'll do it tomorrow

z-adams · 2021-01-24T09:19:08Z

src/adera/Machines/Rocket.cpp

+    DependRes<ShipResourceType> fuel = pkg.get<ShipResourceType>("fuel");
+    if (!fuel.empty())
+    {
+        ActiveEnt fuelSource = static_cast<ActiveEnt>(13);


So this is pretty egregious, but generalizing this stuff is going to take this PR from +500 lines to +700, so perhaps this is acceptable for the time being

what even is this doing? Should this be a search for connected MachineContainers?

I'm just manually assigning the feeding tank to the rocket.

Yes, it should be searching, but since any tank anywhere on the ship can be attached to any rocket anywhere on the ship, it needs a bit of an involved solution. I guess I could give it a config value for the time being and read that, it would just push the hard-codedness to the part config files. I will fix this in the next PR or two, but if you want to change it for now let me know how you think we should handle it. I can include my subsequent commits in this branch too, it'll just make the PR twice as bloated.

src/osp/Resource/PrototypePart.h

z-adams · 2021-01-25T02:31:04Z

src/adera/Machines/Rocket.h

+        float m_massRateFraction;
+        osp::active::ActiveEnt m_sourceEnt;
+    };
+    using fuel_list_t = std::vector<ResourceInput>;


This arrangement kind of sucks, and will be changed in the next PR or two when the connections are automated/generalized

Capital-Asterisk

The biggest problem here is that I'm cringing at my own wiring code, but 10x more.

I also find it quite problematic how SysMachineRocket does the fuel management.

In issue #56 I talk about some 'node' system. Instead of storing states in WireOutput, it should be stored in a separate node component. This can do more than just logic.

All connected fuel containers and consumers can be connected to a common node using this theoretical wiring system, where it acts more like a pressure system instead. When a consumer needs fuel, it writes how much fuel it needs to the node, like negative pressure. This notifies connected fuel tanks which, will try to fill the void and set the node's pressure back to 0.

All connected tanks are known, and requirements from multiple consumers are combined, making it really simple for a system to determine how much fuel to take from each container.

Also consider pumping fuel between tanks, and fuel pipes that connect two different nodes which is making my head hurt right now.

src/adera/Machines/Rocket.cpp

Capital-Asterisk · 2021-01-25T04:02:14Z

src/adera/Machines/Rocket.cpp

+                float massFlow = massFlowRate * m_scene.get_time_delta_fixed();
+                uint64_t required = resource.m_type->resource_quantity(massFlow);
+                MachineContainer* src = m_scene.reg_try_get<MachineContainer>(resource.m_sourceEnt);
+                uint64_t consumed = src->request_contents(required);


I'm not liking how this is writing data into another machine. Where is m_massRateFraction being set from? What happens if one of the tanks are empty?

Good point; it should calculate the mass flow required before withdrawing anything from the tanks. This should probably be combined with the earlier step where I check for fuel availability. I should probably test all this with a bipropellant rocket to make sure it works in general. We can discuss having a node intermediate between fuel sources/sinks elsewhere, I do like that idea.

As for m_massRateFraction, MachineRocket stores a vector m_resourceLines which lists all the types of fuel demanded by the engine, and the by-mass mixture ratio of each one. massFlowRate is the total fuel mass flow rate from earlier, multiplied by the mass mixture ratio to get the mass flow rate of the current resource being iterated over.

Capital-Asterisk · 2021-01-25T04:05:50Z

src/adera/Machines/Rocket.cpp

+    DependRes<ShipResourceType> fuel = pkg.get<ShipResourceType>("fuel");
+    if (!fuel.empty())
+    {
+        ActiveEnt fuelSource = static_cast<ActiveEnt>(13);


what even is this doing? Should this be a search for connected MachineContainers?

Capital-Asterisk · 2021-01-25T04:14:05Z

src/adera/ShipResources.cpp


 using namespace osp;
 using namespace osp::active;
 using namespace adera::active::machines;

+/* ShipResourceType */
+
+double ShipResourceType::resource_volume(uint64_t quantity) const


Many of these functions look constexpr-able. I remember a generalized 2pow bitshift function being mentioned somewhere.

i agree that if you're always using a multiple of two, you could create a constexpr bitshift function for that.

src/adera/ShipResources.cpp

Capital-Asterisk · 2021-01-25T04:30:45Z

src/osp/Active/SysVehicle.cpp

+        /* Deferred machine instantiation
+        * 
+        * Since machines may depend on the existence of other objects in the
+        * part's hierarchy, machine instantiation must be deferred until the
+        * entire hierarchy exists. For instance, a rocket engine's root
+        * "MachineRocket" machine may depend on the existence of a child node
+        * to receive an "ACompExhaustPlume" component that handles graphical
+        * effects related to the rocket machine, but that child node would not
+        * exist in time for the MachineRocket to be instantiated alongside
+        * the root part entity.
+        */
+        part_instantiate_machines(partEntity, machineMapping, proto, partBp);


This sounds good. I recently de-spaghettified SysVehicle in my own branch, making lots of functions static like in my other PRs. There will be merge conflict for sure soon.

src/osp/Active/SysVehicle.cpp

jonesmz · 2021-01-26T04:29:29Z

src/adera/Machines/Rocket.cpp

+    {
+        inputs.push_back(&resource.m_lineIn);
+    }
+    return inputs;*/


prefer

#if 0 #endif

for commenting out code.

or #if false

Really? Why?

Much easier for a lot of code editors to handle gracefully, in addition to not having weird behavior when it comes to nested comments.

src/adera/Machines/Rocket.cpp

jonesmz · 2021-01-26T04:37:38Z

src/adera/Machines/Rocket.h

-inline MachineRocket::MachineRocket(float thrust)
- : Machine(true)
- , m_thrust(thrust)
+inline MachineRocket::MachineRocket(Parameters params, fuel_list_t& resources)


don't take by l-value reference if you are going to unconditionally std::move. Just take the parameter by value in that case.

jonesmz · 2021-01-26T04:40:16Z

src/adera/ShipResources.cpp

+uint64_t ShipResourceType::resource_capacity(double volume) const
+{
+    double units = volume / m_volume;
+    double quantaPerUnit = std::pow(2.0, m_quanta);


if you're always turning m_quanta into 2^m_quanta, why even have m_quanta?

It's tied to the number of bits used to represent the base unit of resource. You could make an enum or something instead but you can't just pick any old quantaPerUnit.

I will concede though that the language is really confusing in all of this stuff. "Quanta" and "unit" are both super ambiguous words in this context; "quanta" might have to change so that it's easier to differentiate the "unit" 1 and the arbitrary unit chosen by the user.

Hmm, now that I think of it, m_quanta is there to increase the precision of the other parameters. If m_quanta is decremented, then the mass, volume, and density can be divided by two to compensate. Though it is pretty useful to describe resources as "1.0m^3 of LOX is 1141.0kg, the game specifically divides this into 65536 units."

m_quanta can just be stored as some unsigned int instead, and is enforced to be a power of two.

A better name might be long but more obvious one: "quantaPerOneUnit"

src/adera/ShipResources.cpp

src/osp/Active/SysVehicle.cpp

src/osp/Active/SysVehicle.h

src/osp/Resource/blueprints.cpp

src/test_application/main.cpp

Capital-Asterisk

I'd say it's best to wait until the new wiring system is implemented before making any major changes, so I'll approve this for now.

The ability for sub-part objects to own their own machines has a number of benefits and will facilitate future features like integral RCS thrusters, or realistic resource mass distribution. Upon import, parts are now recursively scanned for machines and PrototypeMachines are stored in PrototypeObjects instead of PrototypePart. BlueprintObjects also store BlueprintMachines, which are used for unique configuration options, e.g. specifying the type or quantity of fuel in a tank, choosing whether a piece of equipment is retracted or extended initially, etc. This commit also sets up a bare minimum fuel consumption situation where all container machines are automatically filled with a generic "fuel", and all rockets are automatically connected to the large fuel tank (the vessel is currently searched for the first non-empty fuel tank). Eventually some sort of blueprint system will allow for the programmatic connecting of fuel tank to engines. The engine configuration parameters (currently just max thrust and specific impulse) are read in and used to compute the fuel mass burn rate.

z-adams requested review from jonesmz and Capital-Asterisk January 24, 2021 09:05

z-adams commented Jan 24, 2021

View reviewed changes

src/osp/Active/SysVehicle.cpp Outdated Show resolved Hide resolved

z-adams commented Jan 24, 2021

View reviewed changes

src/osp/Resource/PrototypePart.h Show resolved Hide resolved

z-adams commented Jan 25, 2021

View reviewed changes

Capital-Asterisk reviewed Jan 25, 2021

View reviewed changes