Added a checking bad phrase structure #1555

ChugunovRoman · 2023-12-06T12:39:42Z

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 · 2023-12-06T12:42:19Z

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

ChugunovRoman · 2023-12-06T12:52:29Z

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

src/xrGame/PhraseDialog.cpp

Xottab-DUTY · 2023-12-07T00:17:34Z

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

ChugunovRoman · 2023-12-07T07:26:15Z

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

I returned the VERIFY

AMS21 · 2023-12-08T09:58:50Z

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 · 2023-12-09T09:59:00Z

@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 · 2023-12-09T10:38:22Z

@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 · 2023-12-09T11:40:37Z

But why if (!_vertex) check is removed?

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

AMS21 · 2023-12-09T16:07:19Z

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.

ChugunovRoman added Enhancement Code Quality Modmaker Experience Modmaker experience with OpenXRay labels Dec 6, 2023

ChugunovRoman requested a review from Xottab-DUTY December 6, 2023 12:39

ChugunovRoman assigned Xottab-DUTY Dec 6, 2023

ChugunovRoman force-pushed the impr-verify-phrase-graph branch from 185ec2e to 495bbe4 Compare December 6, 2023 12:51

AMS21 reviewed Dec 6, 2023

View reviewed changes

src/xrGame/PhraseDialog.cpp Show resolved Hide resolved

ChugunovRoman requested a review from AMS21 December 6, 2023 16:24

ChugunovRoman force-pushed the impr-verify-phrase-graph branch from 495bbe4 to c2d4f9e Compare December 6, 2023 18:45

ChugunovRoman force-pushed the impr-verify-phrase-graph branch from c2d4f9e to 6e13116 Compare December 7, 2023 07:25

xrGame/PhraseDialog.cpp: Added a checking bad phrase structure

6311e26

ChugunovRoman force-pushed the impr-verify-phrase-graph branch from 6e13116 to 6311e26 Compare December 9, 2023 11:42

Xottab-DUTY force-pushed the dev branch 2 times, most recently from 143d1ca to 118d39d Compare December 16, 2023 10:39

Xottab-DUTY force-pushed the dev branch from b0ba60a to 289f78b Compare February 7, 2024 17:13

Xottab-DUTY force-pushed the dev branch 2 times, most recently from 5b2ec76 to 6fffce9 Compare May 4, 2024 03:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a checking bad phrase structure #1555

Added a checking bad phrase structure #1555

ChugunovRoman commented Dec 6, 2023

ChugunovRoman commented Dec 6, 2023

ChugunovRoman commented Dec 6, 2023

Xottab-DUTY commented Dec 7, 2023

ChugunovRoman commented Dec 7, 2023 •

edited

AMS21 commented Dec 8, 2023

ChugunovRoman commented Dec 9, 2023

Xottab-DUTY commented Dec 9, 2023

ChugunovRoman commented Dec 9, 2023

AMS21 commented Dec 9, 2023 •

edited

Added a checking bad phrase structure #1555

Are you sure you want to change the base?

Added a checking bad phrase structure #1555

Conversation

ChugunovRoman commented Dec 6, 2023

ChugunovRoman commented Dec 6, 2023

ChugunovRoman commented Dec 6, 2023

Xottab-DUTY commented Dec 7, 2023

ChugunovRoman commented Dec 7, 2023 • edited

AMS21 commented Dec 8, 2023

ChugunovRoman commented Dec 9, 2023

Xottab-DUTY commented Dec 9, 2023

ChugunovRoman commented Dec 9, 2023

AMS21 commented Dec 9, 2023 • edited

ChugunovRoman commented Dec 7, 2023 •

edited

AMS21 commented Dec 9, 2023 •

edited