Task core components #860
Task core components #860
Conversation
Add unit tests. Remove UnitCsvFactory reliance on VSFileSystem for better separation of concerns. Game doesn't build because unit tests require VSFileSystem and that pulls in half the game with it. As this is imported from the original lib component branch by file, some stuff here is for future commits.
use of designated initializers requires at least '/std:c++20'
|
|
||
| class Component | ||
| { | ||
| protected: |
There was a problem hiding this comment.
just to note, we should put public information first, then protected, then private. This helps with ABI compatibility, not that we're really concerned about that right now.
There was a problem hiding this comment.
For some reason I assumed the reverse. I'll fix this here, though I can't promise for all of my work.
There was a problem hiding this comment.
Changing things changes the offsets. Keeping public data first keeps the offsets to it the same, and new public goes at the end, pushing the protected and private data further away from the zero point.
If we really wanted a public binary ABI then we'd look at doing another method (what Qt does), but it's not really important just good habit for when it is.
There was a problem hiding this comment.
So, I dug deeper and while most agree on public/protected/private order, fields/constructors/methods is less so.
Since most of the code is fields/constructors/methods, I'll stick with that but reverse the visibility as I go along as these are already not consistent.
|
|
||
| #include "energy_consumer.h" | ||
|
|
||
| double EnergyConsumer::simulation_atom_var = 0.1; |
There was a problem hiding this comment.
could we get some comments on why we're having another simulation_atom_var?
There was a problem hiding this comment.
I'm not sure if you want this in the code or just here - the issue is one of encapsulation. With the work done on the various libraries - resource, components, damage, it's become clear that we need much better separation of concerns.
simulation_atom_var is pervasive throughout the game. However, I need a version of it for consumers. This was my solution.
Ideally, I'd like to create a new library - core. It should have some important constants and the vector class. Maybe with Resource class as well. However, I prefer to finish lib component before tackling something else.
There was a problem hiding this comment.
I think this has the potential to cause a lot of confusion, having two (or more) different instantiations of simulation_atom_var in different scopes. But I suppose we can leave it for now. Approving.
There was a problem hiding this comment.
@royfalk thanks for the explanation; it'd probably be good to have some documentation about it in the code; but not going to require it for this PR to go through. Would you please file a follow-up PR with a small comment about it?
There was a problem hiding this comment.
I do like your idea of the follow-on to have a core library that way too, BTW, and certainly agree we need to take a calculated, stepped approach to getting there.
Reverse order of visibility in EnergyConsumer. Make visibility modifiers explicit in EnergyContainer and Reactor.
|
If no one has any more comments and my answers satisfy, it is ready for merge. |
|
@BenjamenMeyer did you still want to review this PR in more detail? Or is it ready for merge? |
|
I'll try to take a look tonight |
This is part of the lib component tasks.
This commit introduces Component, EnergyConsumer, EnergyContainer and Reactor.
These are not yet connected to the engine. Please focus on reviewing the classes themselves, as they are fairly major.
Please answer the following:
Code Changes:
Issues: