[PATCH] 0ad crash when enabling "smoothing LOS" during game

[pointhi]

If I set "smoothing LOS" in the settings, and start a map like Acropolis Bay, 0 A.D. crash and make a memory access error.

I'm working on Ubuntu 14.04 with an intel graphic card. The Bug appear on the current dev-version

[Josh]

Do you have GLSL enabled?

[pointhi]

I have tested it again, the bug appear only, when I activate "smoothing LOS" while the game is running. It is regardless of whether GLSL is enabled or not.

#2505

[historic_bruno]

It seems this option wasn't designed to be dynamically changed, for example, some required init is done in the CLOSTexture constructor, which only happens at the start of a game. It will need some redesign to solve this. I would recommend removing the option from the menu, at least in-game, for A16.

[historic_bruno]

Related: #2561

[Josh]

I can confirm the crash. It would be nice to cc whoever implemented smooth LOS.

[historic_bruno]

myconid is no longer an active contributor.

[sanderd17]

There are multiple settings like this (that require a restart). It would be nice if those settings could only be applied on restart, and users are asked to restart.

Of course, moving settings from "requiring restart" to "not requiring restart" shouldn't be too hard. As someone might fix the initialisation.

[Josh]

I would think it would be possible to restart more parts of the render to correctly initilize certain settings. If I remember correctly, when changing any setting we already restart most of the renderer.

[dan_trev]

Patch to fix the LOS crash. I have no idea if the LOS is working because I don't know what it looks like!

Hopefully I didn't murder the patch while hand editing the fluff out of it...

[historic_bruno]

Hi, please follow the procedures on SubmittingPatches.

LOS is line of sight, which is what your units/buildings are seeing; smooth LOS is nicer looking transitions when the LOS changes. It's a GLSL effect, so you will need the advanced/GLSL renderer settings enabled to use it.

[dan_trev]

Thanks historic_bruno. I did use SVN diff and the removed the unneeded blank line with a tab removal.

[Josh]

Replying to dan@…:

Thanks historic_bruno. I did use SVN diff and the removed the unneeded blank line with a tab removal.

I think he was more referring to combining your patches into a single patch. There's plenty of information on the internet on how to get a single diff from multiple files. Thanks for the effort so far.

[dan_trev]

Ahhh. OK, I was unsure of that part. I have created a single patch and put that up. Can attachments be deleted?

[dan_trev]

Patch to fix LOS segfault crash when added during game play.

[Josh]

I deleted your earlier patches. I don't have much time, but on the surface it looks mostly okay. Hopefully someone else on the team will be able to take a deeper look soon.

[dan_trev]

I can confirm that the patched svn trunk compiles and runs on Debian Jessie. The LOS appears to activate and deactivate correctly. Linux Daniel01 3.14-2-amd64 #1 SMP Debian 3.14.15-2 (2014-08-09) x86_64 GNU/Linux

[dan_trev]

Replying to Josh:

I deleted your earlier patches. I don't have much time, but on the surface it looks mostly okay. Hopefully someone else on the team will be able to take a deeper look soon.

Thanks, I don't know anyone who does have much time...

[Josh]

Replying to dan@…:

Replying to Josh:

I deleted your earlier patches. I don't have much time, but on the surface it looks mostly okay. Hopefully someone else on the team will be able to take a deeper look soon.

Thanks, I don't know anyone who does have much time...

It would be good to note that it's either very late at night or early in the morning right now for most of the team.

[historic_bruno]

The patch fixes the bug for me on Windows 7, just needs a code review now.

[scythetwirler]

Just took a quick glance. :P

Line 76 - missing a space in comment.

[Itms]

Ok, so for a really late code review:

• what scythetwirler said
• your wasShaderInit name is quite unclear and needs a m_ because it's a member variable. I'd personally call it m_ShaderInitialized or something like this.
• lines 84->92 should work with an early return
• The comment l136 is useless
• Your new comments at the end of the patch are quite unclear to me, and I assume people working with GL shaders wouldn't need them, so maybe remove them.

Thanks a lot for working on this! :D

[Itms]

In 15841:

Allow setting the "Smoothing LoS" option during a game.

Patch by dan_trev, fixes #2513

[Itms]

Hello dan, I committed the patch after fancying it up a bit, because we wanted to have this in A17 and we freeze commits tonight.

Thanks a lot for the patch, looking forward future contributions! :)

[dan_trev]

No problem! I haven't been able to commit but am looking forward to working more on it when time frees up!