Lux-AI-Challenge / Lux-Design-S1

Home to the design and engine of the @Lux-AI-Challenge Season 1, hosted on @kaggle
https://lux-ai.org/
Apache License 2.0
897 stars 152 forks source link

Actually fix Issue #56 and add include guards #66

Closed Desperationis closed 3 years ago

Desperationis commented 3 years ago

Issue #56 was caused by multiple redefinition of internal lux/ files

when importing a header / cpp file combo that both contain lux/kit.hpp. Specifically, it was caused by static variable declarations in lux_io.cpp and constants.cpp and the operator << function in position.hpp.

To fix this, simply inline those functions and variable declarations. The only downside to this is that the project must now be compiled with C++ 17 in order for this to work. Optionally, you can also just simply define these functions and variables in a cpp file and compile it with compile.sh. That would require reworking most of the files in lux though as you would need to change other files into cpp.

In addition to this, add include guards to every single header file in lux/ for the sake of good practice.

Desperationis commented 3 years ago

I'm working on the alternate version for solving this problem without C++17, wait a moment.

Desperationis commented 3 years ago

Alright, I have finished an alternate fix for this issue by creating a single source file and tweaking some headers, thus ending the requirement of C++17. If you prefer the C++17 version, simply revert my second commit.

StoneT2000 commented 3 years ago

thanks for the changes!

StoneT2000 commented 3 years ago

just ran the test suite and i get a compilation error @Desperationis

     AgentCompileError: A compile time error occured. Compile step for agent 1 exited with code: 1; Compiling ./kits/cpp/simple/main.cpp; Compile Output:
Undefined symbols for architecture x86_64:
  "kit::INPUT_CONSTANTS::CITY_TILES", referenced from:
      kit::Agent::update() in main-ea9990.o
  "kit::INPUT_CONSTANTS::RESEARCH_POINTS", referenced from:
      kit::Agent::update() in main-ea9990.o
  "kit::INPUT_CONSTANTS::CITY", referenced from:
      kit::Agent::update() in main-ea9990.o
  "kit::INPUT_CONSTANTS::DONE", referenced from:
      kit::Agent::update() in main-ea9990.o
  "kit::INPUT_CONSTANTS::ROADS", referenced from:
      kit::Agent::update() in main-ea9990.o
  "kit::INPUT_CONSTANTS::UNITS", referenced from:
      kit::Agent::update() in main-ea9990.o
  "kit::INPUT_CONSTANTS::RESOURCES", referenced from:
      kit::Agent::update() in main-ea9990.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

At first glance not sure is causing it, thoughts?

Desperationis commented 3 years ago

I'm pretty sure it's because you didn't compile lux/define.cpp with main.cpp, which is the source file I added that allowed me to compile without C++17. It contains the definitions for all of those

Desperationis commented 3 years ago

I'm not sure what compiler you're using, but if it's emcc, just add lux/define.cpp right after main.cpp. You can see the format of the command in command.bat in the changes I made in the v1.1.x branch

Desperationis commented 3 years ago

I'm even more confident in that guess. I was able to replicate your error by running g++ main.cpp and got the same error.

/usr/bin/ld: /tmp/ccubrGor.o: in function `kit::Agent::update()':
main.cpp:(.text._ZN3kit5Agent6updateEv[_ZN3kit5Agent6updateEv]+0xb8): undefined reference to `kit::INPUT_CONSTANTS::DONE[abi:cxx11]'
/usr/bin/ld: main.cpp:(.text._ZN3kit5Agent6updateEv[_ZN3kit5Agent6updateEv]+0x185): undefined reference to `kit::INPUT_CONSTANTS::RESEARCH_POINTS[abi:cxx11]'
/usr/bin/ld: main.cpp:(.text._ZN3kit5Agent6updateEv[_ZN3kit5Agent6updateEv]+0x229): undefined reference to `kit::INPUT_CONSTANTS::RESOURCES[abi:cxx11]'
/usr/bin/ld: main.cpp:(.text._ZN3kit5Agent6updateEv[_ZN3kit5Agent6updateEv]+0x34e): undefined reference to `kit::INPUT_CONSTANTS::UNITS[abi:cxx11]'
/usr/bin/ld: main.cpp:(.text._ZN3kit5Agent6updateEv[_ZN3kit5Agent6updateEv]+0x63f): undefined reference to `kit::INPUT_CONSTANTS::CITY[abi:cxx11]'
/usr/bin/ld: main.cpp:(.text._ZN3kit5Agent6updateEv[_ZN3kit5Agent6updateEv]+0x7d3): undefined reference to `kit::INPUT_CONSTANTS::CITY_TILES[abi:cxx11]'
/usr/bin/ld: main.cpp:(.text._ZN3kit5Agent6updateEv[_ZN3kit5Agent6updateEv]+0xa2c): undefined reference to `kit::INPUT_CONSTANTS::ROADS[abi:cxx11]'
collect2: error: ld returned 1 exit status

However, when I ran g++ main.cpp lux/define.cpp, I successfully compiled.

StoneT2000 commented 3 years ago

ah i see okay this will cause some problems with people running the C++ bot directly instead of transpiling first. This means the engine will actually just call g++ main.cpp to compile and then try to run the bot

Is there a way to get it such that g++ main.cpp is sufficient and will work?

Desperationis commented 3 years ago

There is a way, though I don't know if you'll like it. You can simply move the contents of define.cpp somewhere in main.cpp like this and g++ main.cpp will work with no issues. It's either this or assume the person has C++17 installed so that all that stuff can be hidden in lux using inline static variables

#include "lux/kit.hpp"
#include <string.h>
#include <vector>
#include <set>
#include <stdio.h>

namespace kit
{
  using namespace std;
  string INPUT_CONSTANTS::DONE = "D_DONE";
  string INPUT_CONSTANTS::RESOURCES = "r";
  string INPUT_CONSTANTS::RESEARCH_POINTS = "rp";
  string INPUT_CONSTANTS::UNITS = "u";
  string INPUT_CONSTANTS::CITY = "c";
  string INPUT_CONSTANTS::CITY_TILES = "ct";
  string INPUT_CONSTANTS::ROADS = "ccd";
}

namespace lux
{

    ostream &operator<<(ostream &out, const Position &p)
    {
        out << "(" << p.x << "," << p.y << ")"; // access private data
        return out;
    };
}

using namespace std;
using namespace lux;
int main()
{
  kit::Agent gameState = kit::Agent();
  // initialize
  gameState.initialize();

  while (true)
  {
    /** Do not edit! **/
    // wait for updates
    gameState.update();

(and so on)
Desperationis commented 3 years ago

And you can run the bot directly??? How does that work with the visualizer, or is it just for debug purposes?

StoneT2000 commented 3 years ago

let me look into this a bit deeper. I feel there should be a easy solution here that doesn't require having to put the constants into main

StoneT2000 commented 3 years ago

@Desperationis whats the use case for compiling 2 files actually? As opposed to just compiling main.cpp (and having it include foo instead)

StoneT2000 commented 3 years ago

Here's what we encourage competitors to do instead

In main.cpp, add your helper files and utilities etc.

#include "lux/kit.hpp"
#include <string.h>
#include <vector>
#include <set>
#include <stdio.h>
#include "foo.cpp"

In foo.hpp, we might have

#include "lux/kit.hpp"
void foo();

And in foo.cpp

#include "lux/kit.hpp"
#include "foo.hpp"

void foo() {
  int i = 1 + 1;
}
StoneT2000 commented 3 years ago

If you don't quite like this way of doing it, by all means feel free to change up the kit for yourself (as long as it works and runs your golden). I'll make sure to refer others to this issue if they would like to pursue this method.

Desperationis commented 3 years ago

@StoneT2000 The use case I'm looking for in a header / cpp pair is solving circular dependency. I have a class, lets say WorkerFinder, that finds workers, so it includes their hpp file Worker.h. However, I want workers to find other workers, so Worker.h includes WorkerFinder.h. Using just header files, this configuration is impossible unless WorkerFinder provides a forward declaration of Worker.h and then is implemented in a separate cpp file.

Also, including cpp files is bad practice because if you do it wrong you get the same issue that caused this whole mess. In this case, your completely fine JUST including foo.cpp in main.cpp, though it really isn't ideal. If you say so, I'll just keep a local branch of the cpp kit with these changes that will allow cpp files to be compiled alongside main.cpp

StoneT2000 commented 3 years ago

Ah I see, that makes sense. Wait would it just work fine if we just take out all the definitions from the kit hpp files and move them to cpp files (correct me if my wrong, my c++ is rusty)

For starter kit I'd still prefer to keep it simple where people would not need to edit much of the kit or the compilation scripts and only need to worry about new files they make and main.cpp

Desperationis commented 3 years ago

Yeah you're pretty much spot on. If you move all definitions in the kit hpp files into a single or multiple source files and compile them, you solve the issue. The changes I made do exactly that but only for the definitions that cause problems.

I'm not too sure on this but you could theoretically just include define.cpp just once in main.cpp and it'll solve the issue. I'll try that right now and see how it goes

Desperationis commented 3 years ago

Yeah no, just including define.cpp into main.cpp once doesn't work since you still have to deal with lux/ files

Edit: I am wrong :(

Desperationis commented 3 years ago

try that, I tried g++ main.cpp and it worked with no issue. Only downside is that you have to include literally one more header on main.cpp only but yeah

StoneT2000 commented 3 years ago

This looks good now, merging :)