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

Added a checking bad phrase structure #1555

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ChugunovRoman
Copy link
Contributor

This verify checks for an already existing phrase added with the same ID, which is an incorrect dialog structure. Each new phrase must have a unique identifier. This check is simply more informative than just m_edges.end() == std::find(m_edges.begin(), m_edges.end(), vertex->vertex_id()). The following crash will immediately follow:

FATAL ERROR
 
[error] Expression    : m_edges.end() == std::find(m_edges.begin(), m_edges.end(), vertex->vertex_id())
[error] Function      : add_edge
[error] File          : /home/ruut/Documents/qemu_shared/xray-16/src/xrAICore/Navigation/graph_vertex_inline.h
[error] Line          : 68
[error] Description   : assertion failed
 

stack trace:

xrDebug::GatherInfo(char*, unsigned long, ErrorLocation const&, char const*, char const*, char const*, char const*)
xrDebug::Fail(bool&, ErrorLocation const&, char const*, char const*, char const*, char const*)
CGraphVertex<CPhrase*, shared_str, CGraphAbstract<CPhrase*, float, shared_str, Loki::EmptyType> >::add_edge(CGraphVertex<CPhrase*, shared_str, CGraphAbstract<CPhrase*, float, shared_str, Loki::EmptyType> >*, float const&)
CGraphAbstract<CPhrase*, float, shared_str, Loki::EmptyType>::add_edge(shared_str const&, shared_str const&, float const&)
CPhraseDialog::AddPhrase(char const*, shared_str const&, shared_str const&, int)
CPhraseDialog::AddPhrase_script(char const*, char const*, char const*, int)
luabind::detail::invoke_struct<luabind::meta::type_list<>, luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int)>::call_struct<true, false, luabind::meta::index_list<0u, 1u, 2u, 3u, 4u> >::call(lua_State*, CPhrase* (CPhraseDialog::*&)(char const*, char const*, char const*, int), std::tuple<luabind::default_converter<CPhraseDialog&, void>, luabind::default_converter<char const*, void>, luabind::default_converter<char const*, void>, luabind::default_converter<char const*, void>, luabind::default_converter<int, void> >&)
luabind::detail::invoke_struct<luabind::meta::type_list<>, luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int)>::invoke(lua_State*, luabind::detail::function_object const&, luabind::detail::invoke_context&, CPhrase* (CPhraseDialog::*&)(char const*, char const*, char const*, int))
luabind::detail::function_object_impl<CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int), luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, luabind::meta::type_list<> >::invoke_defer(lua_State*, luabind::detail::function_object_impl<CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int), luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, luabind::meta::type_list<> >*, luabind::detail::invoke_context&, int&)
luabind::detail::function_object_impl<CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int), luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, luabind::meta::type_list<> >::entry_point(lua_State*)
/home/ruut/Documents/qemu_shared/xray-16/bin/x64/Debug/xrLuaJIT.so(+0xa3d5) [0x7f51526a73d5]
/home/ruut/Documents/qemu_shared/xray-16/bin/x64/Debug/xrLuaJIT.so(lua_pcall+0xae) [0x7f51526bab5e]
luabind::detail::pcall(lua_State*, int, int)
void luabind::detail::call_function_struct<void, luabind::meta::type_list<>, luabind::meta::index_list<1u>, 1u, &luabind::detail::pcall, true>::call<CPhraseDialog*>(lua_State*, CPhraseDialog*&&)
void luabind::call_pushed_function<void, luabind::meta::type_list<>, CPhraseDialog*>(lua_State*, CPhraseDialog*&&)
void luabind::call_function<void, luabind::meta::type_list<>, CPhraseDialog*>(luabind::adl::object const&, CPhraseDialog*&&)
CPhraseDialog::load_shared(char const*)
CSharedClass<SPhraseDialogData, shared_str, false>::load_shared(shared_str, char const*)
CPhraseDialog::Load(shared_str)
CPhraseDialogManager::AddAvailableDialog(shared_str, CPhraseDialogManager*)

The new verify:

 
FATAL ERROR
 
[error] Expression    : !_vertex
[error] Function      : AddPhrase
[error] File          : /home/ruut/Documents/qemu_shared/xray-16/src/xrGame/PhraseDialog.cpp
[error] Line          : 259
[error] Description   : Dublicate phrase ID: [244] for phrase: [dm_do_not_know_10]. Existed phrase by this ID: [dm_no_more_8]
 

stack trace:

xrDebug::GatherInfo(char*, unsigned long, ErrorLocation const&, char const*, char const*, char const*, char const*)
xrDebug::Fail(bool&, ErrorLocation const&, char const*, char const*, char const*, char const*)
xrDebug::Fail(bool&, ErrorLocation const&, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char const*, char const*)
CPhraseDialog::AddPhrase(char const*, shared_str const&, shared_str const&, int)
CPhraseDialog::AddPhrase_script(char const*, char const*, char const*, int)
luabind::detail::invoke_struct<luabind::meta::type_list<>, luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int)>::call_struct<true, false, luabind::meta::index_list<0u, 1u, 2u, 3u, 4u> >::call(lua_State*, CPhrase* (CPhraseDialog::*&)(char const*, char const*, char const*, int), std::tuple<luabind::default_converter<CPhraseDialog&, void>, luabind::default_converter<char const*, void>, luabind::default_converter<char const*, void>, luabind::default_converter<char const*, void>, luabind::default_converter<int, void> >&)
luabind::detail::invoke_struct<luabind::meta::type_list<>, luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int)>::invoke(lua_State*, luabind::detail::function_object const&, luabind::detail::invoke_context&, CPhrase* (CPhraseDialog::*&)(char const*, char const*, char const*, int))
luabind::detail::function_object_impl<CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int), luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, luabind::meta::type_list<> >::invoke_defer(lua_State*, luabind::detail::function_object_impl<CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int), luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, luabind::meta::type_list<> >*, luabind::detail::invoke_context&, int&)
luabind::detail::function_object_impl<CPhrase* (CPhraseDialog::*)(char const*, char const*, char const*, int), luabind::meta::type_list<CPhrase*, CPhraseDialog&, char const*, char const*, char const*, int>, luabind::meta::type_list<> >::entry_point(lua_State*)
/home/ruut/Documents/qemu_shared/xray-16/bin/x64/Debug/xrLuaJIT.so(+0xa3d5) [0x7f51526a73d5]
/home/ruut/Documents/qemu_shared/xray-16/bin/x64/Debug/xrLuaJIT.so(lua_pcall+0xae) [0x7f51526bab5e]
luabind::detail::pcall(lua_State*, int, int)
void luabind::detail::call_function_struct<void, luabind::meta::type_list<>, luabind::meta::index_list<1u>, 1u, &luabind::detail::pcall, true>::call<CPhraseDialog*>(lua_State*, CPhraseDialog*&&)
void luabind::call_pushed_function<void, luabind::meta::type_list<>, CPhraseDialog*>(lua_State*, CPhraseDialog*&&)
void luabind::call_function<void, luabind::meta::type_list<>, CPhraseDialog*>(luabind::adl::object const&, CPhraseDialog*&&)
CPhraseDialog::load_shared(char const*)
CSharedClass<SPhraseDialogData, shared_str, false>::load_shared(shared_str, char const*)
CPhraseDialog::Load(shared_str)
CPhraseDialogManager::AddAvailableDialog(shared_str, CPhraseDialogManager*)
CActor::UpdateAvailableDialogs(CPhraseDialogManager*)

@ChugunovRoman
Copy link
Contributor Author

@Xottab-DUTY But I’m not sure if this check should be removed from the MASTER_GOLD?

@ChugunovRoman
Copy link
Contributor Author

Ah, I forgot that checks are disabled in MASTER_GOLD...

@Xottab-DUTY
Copy link
Member

@ChugunovRoman, please, make it not crash. Prefer robustness and always try not to crash instead of using R_ASSERT.

@ChugunovRoman
Copy link
Contributor Author

ChugunovRoman commented Dec 7, 2023

@ChugunovRoman, please, make it not crash. Prefer robustness and always try not to crash instead of using R_ASSERT.

I returned the VERIFY

@AMS21
Copy link
Contributor

AMS21 commented Dec 8, 2023

I returned the VERIFY

I believe what @Xottab-DUTY is trying to say that it would be even better if we didn't crash at all. So instead of an VERIFY or R_ASSERT we would have something like this:

if (_vertex)
{
    Msg(make_string("! Dublicate phrase ID: [%s] for phrase: [%s]. Existed phrase by this ID: [%s]", phrase_id.c_str(), text, _vertex->data()->GetText()));
    return;
}

Which would give mod makers and player a better message to understand what exactly the problem is and where but not crash the game.

Although you may need to change to function to return a bool and also add an early return in the calling functions.

@ChugunovRoman
Copy link
Contributor Author

@AMS21 There will be no crash in the release build. Because VERIFY is used. And in the debug assembly needs such crash because, allow you to catch an error in a timely manner and fix it. I would to keep the VERIFY.
@Xottab-DUTY Do you agree with me?

@Xottab-DUTY
Copy link
Member

@AMS21 There will be no crash in the release build. Because VERIFY is used. And in the debug assembly needs such crash because, allow you to catch an error in a timely manner and fix it. I would to keep the VERIFY. @Xottab-DUTY Do you agree with me?

But why if (!_vertex) check is removed?

@ChugunovRoman
Copy link
Contributor Author

But why if (!_vertex) check is removed?

Now if the game doesn’t crash, it’s probably worth returning this check.

@AMS21
Copy link
Contributor

AMS21 commented Dec 9, 2023

I don't think my point here is really getting across, but it's not too important and might also be a me problem.

Sorry for all the confusion 😓

I do think in essence this is a good addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Quality Enhancement Modmaker Experience Modmaker experience with OpenXRay
Projects
Status: To do
Development

Successfully merging this pull request may close these issues.

3 participants