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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sector Map: Auto Route button crashes game when no hyperdrive fitted #4667

Open
craigo- opened this issue Aug 16, 2019 · 9 comments

Comments

@craigo-
Copy link
Contributor

commented Aug 16, 2019

Observed behaviour

With a ship that is not fitted with a hyperdrive, pressing the "Auto Route" button on the sector map results in a game crash:

image

Expected behaviour

That the game doesn't crash 馃槃

Perhaps for ships without a hyperspace drive, pressing the "Auto Route" button could generate an informational alert (e.g. "No hyperdrive"), or perhaps simply do nothing at all.

Steps to reproduce

  1. Start at Barnard's Star (Xylophis, no hyperdrive)
  2. Switch to Sector Map
  3. Select any system distant from Barnard's Star
  4. Click "Auto Route"

NB: when doing the above with a hyperdrive-equipped ship, the game does not crash.

My pioneer version (and OS):

Pioneer 20190203, and compiled from master (commit 4bc31ee)

Windows 10 Enterprise 1809 x64

@mike-f1

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

The bug raise in

const float max_range = hyperdrive.CallMethod<float>("GetMaximumRange", Pi::player);

when a GetMaximumRange is searched in an empty table, as the previous line is
getting an engine which is not present.

In particular, when PushValueToStack (in LuaTable.h, line 311) calls lua_gettable, it (eventually) fail.

A fast workaround is to check in Lua if there's an engine, but a better solution may be to put a general mechanism to check if there's a value that should be returned.

What's your thoughts?

Ping @Web-eWorks , @ecraven and @laarmen

@impaktor

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

I assume C++ side shouldn't fail with a crash, but fail with grace, like a ballerina falling on her ass.

@laarmen

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@laarmen

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@mike-f1

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

If that's the case, that's IMO a bug.

馃憤

However, a method to actually check that it is the case, that would be called explicitly,
would be a great addition.

This line:

const ScopedTable hyperdrive = ScopedTable(LuaObject<Player>::CallMethod<LuaRef>(Pi::player, "GetEquip", "engine", 1));

is initializing a ScopedTable from a LuaRef, which already has a IsValid() method.
However, that method seems not working: splitting that line in

	LuaRef lua_ref = LuaObject<Player>::CallMethod<LuaRef>(Pi::player, "GetEquip", "engine", 1);
	if (!lua_ref.IsValid()) return;

	const ScopedTable hyperdrive = ScopedTable(lua_ref);

doesn't prevent the error as a LUA_NOREF is -2 whilst lua_ref.m_id is -1 (here the link)

And here I start having troubles understanding why the above happens and what's the logic behind 馃槦

...If I may suggest something, all the above could be helped with a check on lines:

pioneer/src/LuaTable.h

Lines 310 to 316 in 4bc31ee

template <class Key>
LuaTable LuaTable::PushValueToStack(const Key &key) const
{
pi_lua_generic_push(m_lua, key);
lua_gettable(m_lua, m_index);
return *this;
}

which print if there's no value for the given key improving (or I hope so) debugging...

@laarmen

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@Web-eWorks

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@laarmen maybe I'm reading this wrong, but I don't see a way to directly return a ScopedTable from LuaObject<T>::CallMethod() - not only does CallMethod delete the return value from the stack, but there's no pi_lua_generic_pull overload for ScopedTable or LuaTable for that matter. How are you thinking of rectifying this?

@mike-f1

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Is there an Option type in C++ these days ?

https://en.cppreference.com/w/cpp/utility/optional ...but it needs C++17, which is quite an(other) overkill...

My question is when an "empty object" wrapped in a LuaRef should be considered valid? ...Which is similar to your thought:

"why is the hyperdrive an empty table and not nil"

...because if there's no need for an empty object/table then that is a design (and logic) flaws...

@Web-eWorks , you should consider that that method is working if an hyperdrive is present:
indeed it calls this function in order to retrieve a "valid" LuaRef...

Thus all this issue is about "how to handle error when C++ needs Lua", and "how to crash in your feet" (= giving right hints to devs and useful messages)

@laarmen

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.