Details

source/main.cpp
356 ↗	(On Diff #22153)	I agree with you that it is bether now but it looks like a old signature and readers might think that the `rlInterface ? &*rlInterface : nullptr` is there to conform that old signature.

vladislavbelov added inline comments.Aug 18 2023, 7:57 PM

source/main.cpp

485 ↗

(On Diff #22155)

const CmdLineArgs& args.

673–680 ↗

(On Diff #22155)

Maybe then make it such there is no need of std::optional?

if (args.Has("rl-interface"))
{
	RL::Interface rlInterface{StartRLInterface(args)};
	while (g_Shutdown == ShutdownType::None)
		rlInterface->TryApplyMessage();
}
else
{
	while (g_Shutdown == ShutdownType::None)
		NonVisualFrame();
}

Or if you prefer lambdas (though it seems a bit more complicated) :)

... processFrame = args.Has("rl-interface")
	? [rlInterface=StartRLInterface(args)]() { rlInterface->TryApplyMessage() };
	: []() { NonVisualFrame(); };
while (g_Shutdown == ShutdownType::None)
	processFrame();

phosit added inline comments.Aug 18 2023, 8:33 PM

source/main.cpp
673–680 ↗	(On Diff #22155)	Yes unswitching the loop does make sense. `StartRLInterface` can't be used without `std::optional`. Are you ok if I move the code in `StartRLInterface` to the constructor of `RL::Interface`? The second example doesn't work: lambdas can't be converted to each other. `std::function` could be used but that's ugly.

Unswitch the visual and the non-visual loop. This removes the need for std::optional but now there are four similar loops. I deduplicated the loop in to WhileNoShutdown.

Build failure - The Moirai have given mortals hearts that can endure.

Debug:
     4>e:\jenkins\workspace\vs2015-differential\source\main.cpp(680): error C2248: 'RL::Interface::Interface': cannot access private member declared in class 'RL::Interface' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\pyrogenesis.vcxproj]
     4>e:\jenkins\workspace\vs2015-differential\source\main.cpp(692): error C2248: 'RL::Interface::Interface': cannot access private member declared in class 'RL::Interface' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\pyrogenesis.vcxproj]

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/8306/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/7217/display/redirect

Harbormaster completed remote builds in B22246: Diff 22159.Aug 19 2023, 10:21 AM

I don't think that:

WhileNoShutdown(
	[]{
		Frame();
	});

is more readable/safe than:

while (g_Shutdown == ShutdownType::None)
	Frame();

But I have no strong objections.

source/main.cpp
172–173 ↗	(On Diff #22159)	There is no need to write `Func` instead of `Function`.
678–680 ↗	(On Diff #22159)	Needs more indentation, since it's a function call argument.
673–680 ↗	(On Diff #22155)	I'm ok with that. Yeah, that's why I wrote `...` instead of `auto`.

Yes it's a bit less readable, but it's better then repeating while (g_Shutdown == ShutdownType::None) four times.

source/main.cpp
172–173 ↗	(On Diff #22159)	Now as i think about it it should be `Callback` ;P
678–680 ↗	(On Diff #22159)	IMO, Lambdas don't fit nicely into ower CC. But yea I change it.
673–680 ↗	(On Diff #22155)	Since I don't use `std::optional`, it's not required anymore. Might still be a good thing to do... but not related to this diff.

Rename Func to Callback
Indent the lambdas more

Build failure - The Moirai have given mortals hearts that can endure.

Debug:
     4>e:\jenkins\workspace\vs2015-differential\source\main.cpp(680): error C2248: 'RL::Interface::Interface': cannot access private member declared in class 'RL::Interface' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\pyrogenesis.vcxproj]
     4>e:\jenkins\workspace\vs2015-differential\source\main.cpp(692): error C2248: 'RL::Interface::Interface': cannot access private member declared in class 'RL::Interface' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\pyrogenesis.vcxproj]

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/8307/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/7218/display/redirect

Harbormaster completed remote builds in B22247: Diff 22160.Aug 19 2023, 11:51 AM

I changed my mind about "unswitching". Now i think it's better if the code path are unified (only one loop and switching/branching inside the loop). Bacicly this version
There where two path a visual and a non visual. With the RL::Interface I doubled the number of paths. It doesn't scale. We would have to double the number of paths everytime we introduce a new option {replay/autostart, map editor...}.
In future we should also unify Frame and NonVisualFrame.