Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Easier to model docking - Part 1 #3075

Merged
merged 19 commits into from Sep 10, 2014

Conversation

fluffyfreak
Copy link
Contributor

Description:

The current way of defining how ships dock with a space station is a lot of work, especially when you get more than a handful of docking bays in a station.

This is because you must manually place each locator for the docking and leaving stages for each and every bay. They must be uniquely named, oriented and the whole process is quite painful, error prone and time consuming. The fact that it's actually better than the old-old system is great but not good enough.

This PR changes things so that there is a single entrance locator placed for a group of bays, and then each bay gets a single locator which defines the location and orientation of a docked ship. Almost everything else is then generated by the code to replicate, internally, the existing docking process.

This is considered Part 1 because it keeps most of the existing docking systems in place. The system is automatic once the player/AI has crossed the entrance threshold.
There will be a Part 2 which changes the system so that the player can manually fly the ship into their bay, but this is beyond the scope of this PR.

Current state:

Usable/testable with rough edges and the existing hoop_spacestation is broken.

Newer tags:

There are only 3 tags for a station now:

  • Entrance locator: entrance_port01
  • Pad locators: loc_A001_p01_s0_500_b01
  • Optional Exit locator: exit_port01

There are some important parts like if you've named your entrance entrance_port01 then there must be some pad locators with p01 in the name to use it, i.e: the 01 parts must match.
To keep things simple I would try and use a naming convention starting at 01 and going up for as many ports as you have with a minimum of 1 pad per port.

The pad locator only looks scary so taking loc_A001_p01_s0_500_b01 apart we have:

  • loc_ this is common keep the start the same always.
  • A001_ this a pad name, 4 characters long so John_, #AA#_, etc are also valid.
  • p01_ the port number as mentioned above.
  • s0_500_ the ship size range in this case from 0 to 500.
  • b01 the bay number - keep them sequential, from 1 to 240.

All the other tags that used to be created by hand are now generated internally.

ToDo / WIP:

There are a couple of things still to do.

  • optional tag/locator defined for an alternative exit allowing ships to enter one way but leave via another,
  • I might need to solve the hoop_spacestation problem but ideally it would be solved by remodelling it,
  • testing, testing, testing thoroughly.

Andy

@fluffyfreak fluffyfreak changed the title WIP - Easier to model docking - Part 1 Easier to model docking - Part 1 Jul 30, 2014
@fluffyfreak
Copy link
Contributor Author

big_crappy is dockable, but hoop is disabled due to it's docking being... awkward.

@fluffyfreak fluffyfreak mentioned this pull request Aug 30, 2014
@fluffyfreak
Copy link
Contributor Author

@impaktor, @johnbartholomew would anyone be able to take a look over the code here for obvious issues, style etc?

Testing would be cool too :)

@impaktor
Copy link
Member

impaktor commented Sep 3, 2014

Thanks for the confidence, but I'm afraid I'm very unfamiliar with this code and don't understand much, but I'll gladly give this a test and report back tomorrow.

I've looked through the code, and as an example of my lacking understanding, I don't see the point in using const for a non-reference variable in void SpaceStation::SetDocked(Ship *ship, const int port).

If you want more reviewers, maybe if you scratch @lwho's back (#2773), he will scratch yours. ;)

@fluffyfreak
Copy link
Contributor Author

@impaktor I didn't even notice #2773 needed reviewing :)

I use const excessively, but especially for parameters passed in because it means things coming into a method aren't going to change during it's usage. Too much experience has shown that every time someone re-uses a parameter variable during a method, someone else won't know/notice that and then you've got a bug.

@impaktor
Copy link
Member

impaktor commented Sep 4, 2014

I've tested this a little bit, with the two different ground stations, and the crappy orbital. When landing in London there seems to be a small glitch in the animation just when I'm put on the landing pad, nothing major, but it is the only thing I noted when testing it so I'm mentioning it now. The "glitch" was most noticeable when my ship was facing outward, and then just when I touch down I seem to do a 180 spin in one frame or something. As I said, nothing major.

@fluffyfreak fluffyfreak mentioned this pull request Sep 4, 2014
@fluffyfreak
Copy link
Contributor Author

@impaktor thanks I'll look into that :)
Guess I might not get this merged this week.

@lwho
Copy link
Contributor

lwho commented Sep 5, 2014

I'll have a look at the code. Might take some time though, since I'm also not familiar with this part of the code.

@@ -54,7 +54,8 @@ void SpaceStation::Save(Serializer::Writer &wr, Space *space)
wr.Bool(mBayGroups[i].inUse);
wr.Int32(mBayGroups[i].bayIDs.size());
for (Uint32 j=0; j<mBayGroups[i].bayIDs.size(); j++) {
wr.Int32(mBayGroups[i].bayIDs[j]);
wr.Int32(mBayGroups[i].bayIDs[j].first);
wr.String(mBayGroups[i].bayIDs[j].second);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires s_saveVersion to be bumped, which currently does not happen.

@lwho
Copy link
Contributor

lwho commented Sep 5, 2014

When trying to understanding the code, I get totally confused about what is a port.

In the locator there is a port field which gets set to SBayGroup::port suggesting there is a 1:1 relation between group and port.

However, a group is a group of bays as far as I understand (i.e. group and bays is 1:N). On the other hand there is that m_ports map, which has a port for each bay, so 1:1 for bay:port, resulting in 1:N for group:port. As the cardinality does not match, this suggests to me that this is another kind of port.

@fluffyfreak
Copy link
Contributor Author

Yes the Group:Port:Bay stuff is massively confusing I'll try and tidy that up a bit.

@fluffyfreak
Copy link
Contributor Author

Ok I've tried to clarify the naming. What was called a SPort is now a PortPath, BayGroup becomes Port and a Bay is still a bay :)

The old BayGroup was really a port. For some reason I had named the locators/path that ships follow as Port and then the naming of everything else got all messed up after that.

…ocking locator.

Do a ray-plane intersection to find the place nearest to the pad locator.
The ray comes from the furthest approach point, through the centre line of the station.
The plane is defined by the location of the pad and the normalised axes of it's transform.
@fluffyfreak
Copy link
Contributor Author

I've copied the contents of #3149 into this branch/PR now so it has a working hoop_spacestation again with the new docking tags. Thanks @nozmajner for doing that work!!!

Completely reworked the intermediate docking locator generation and it's now far more reliable and works for hoop_spacestation, big_crappy_spacestation and the upcoming orbital_spacestation.

@impaktor
Copy link
Member

impaktor commented Sep 8, 2014

Nice. I gave it a try.

Landed "manually" if we now can call it that. But works as it used to.

@fluffyfreak
Copy link
Contributor Author

@impaktor and @johnbartholomew ok I plan on merging this along with the other savegame bump PRs later today.

Are there any objections?

@johnbartholomew
Copy link
Contributor

No objection from me, but I haven't tried to test it.

@impaktor
Copy link
Member

👍

fluffyfreak added a commit that referenced this pull request Sep 10, 2014
@fluffyfreak fluffyfreak merged commit d78e170 into pioneerspacesim:master Sep 10, 2014
@fluffyfreak fluffyfreak deleted the better_docking branch September 10, 2014 13:09
@impaktor
Copy link
Member

I'm getting a lot of warnings from SpaceStationType.cpp. Ugh, tried setting terminal locale to US, and I have calender and date clearly as US locale, but for some retarded reason g++ doesn't respect the locale of the terminal, but I assume you can guess the problems, it's all germanic language anyways:

  CXX      SpaceStationType.o
SpaceStationType.cpp: I medlemsfunktion ”void SpaceStationType::OnSetupComplete()”:
SpaceStationType.cpp:92:114: varning: format ”%s” förväntar sig argument av typen ”char*”, men argument 3 har typen ”char (*)[8]” [-Wformat=]
   PiVerify(5 == sscanf(locIter->GetName().c_str(), "loc_%4s_p%d_s%d_%d_b%d", &padname, &portId, &minSize, &maxSize, &bay));
                                                                                                                  ^
In file included from libs.h:59:0,
                 from SpaceStationType.h:7,
                 from SpaceStationType.cpp:4:
matrix4x4.h:26:75: varning: ”intersectionPos.vector3<float>::z” kan användas oinitierad i denna funktion [-Wmaybe-uninitialized]
  void SetTranslate(const vector3<T> &v) { cell[12] = v.x; cell[13] = v.y; cell[14] = v.z; }
                                                                           ^
SpaceStationType.cpp:150:13: anm: ”intersectionPos.vector3<float>::z” deklarerades här
    vector3f intersectionPos;
             ^
In file included from libs.h:59:0,
                 from SpaceStationType.h:7,
                 from SpaceStationType.cpp:4:
matrix4x4.h:26:59: varning: ”intersectionPos.vector3<float>::y” kan användas oinitierad i denna funktion [-Wmaybe-uninitialized]
  void SetTranslate(const vector3<T> &v) { cell[12] = v.x; cell[13] = v.y; cell[14] = v.z; }
                                                           ^
SpaceStationType.cpp:150:13: anm: ”intersectionPos.vector3<float>::y” deklarerades här
    vector3f intersectionPos;
             ^
In file included from libs.h:59:0,
                 from SpaceStationType.h:7,
                 from SpaceStationType.cpp:4:
matrix4x4.h:26:43: varning: ”intersectionPos.vector3<float>::x” kan användas oinitierad i denna funktion [-Wmaybe-uninitialized]
  void SetTranslate(const vector3<T> &v) { cell[12] = v.x; cell[13] = v.y; cell[14] = v.z; }
                                           ^
SpaceStationType.cpp:150:13: anm: ”intersectionPos.vector3<float>::x” deklarerades här
    vector3f intersectionPos;
             ^

Non en_US locale for compiler error is kind of stupid, I know, I've changed my LC_MESSAGES permanently now. Let's see if it works next reboot.

@fluffyfreak
Copy link
Contributor Author

What is it complaining about? The maybe uninitialised I understand but the %s formatting one I don't

@impaktor
Copy link
Member

SpaceStationType.cpp:92:114: Warning: format ”%s” expects arg of type ”char*”, but argument 3 has type ”char (*)[8]” [-Wformat=]
   PiVerify(5 == sscanf(locIter->GetName().c_str(), "loc_%4s_p%d_s%d_%d_b%d", &padname, &portId, &minSize, &maxSize, &bay));

@fluffyfreak
Copy link
Contributor Author

@impaktor #3178 should fix those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants