#2513 closed defect (fixed)
[PATCH] 0ad crash when enabling "smoothing LOS" during game
Reported by: | pointhi | Owned by: | dan_trev |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 17 |
Component: | Core engine | Keywords: | |
Cc: | Patch: |
Description
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
Attachments (1)
Change History (29)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
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.
comment:3 by , 10 years ago
Milestone: | Alpha 16 → Alpha 17 |
---|
comment:4 by , 10 years ago
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.
comment:5 by , 10 years ago
Summary: | 0ad crash when enabling "smoothing LOS" → 0ad crash when enabling "smoothing LOS" during game |
---|
comment:7 by , 10 years ago
I can confirm the crash. It would be nice to cc whoever implemented smooth LOS.
comment:9 by , 10 years ago
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.
comment:10 by , 10 years ago
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.
comment:11 by , 10 years ago
comment:12 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:13 by , 10 years ago
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...
comment:14 by , 10 years ago
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.
comment:15 by , 10 years ago
Keywords: | review patch added |
---|---|
Summary: | 0ad crash when enabling "smoothing LOS" during game → [PATCH] 0ad crash when enabling "smoothing LOS" during game |
follow-up: 17 comment:16 by , 10 years ago
Thanks historic_bruno. I did use SVN diff and the removed the unneeded blank line with a tab removal.
follow-up: 18 comment:17 by , 10 years ago
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.
comment:18 by , 10 years ago
Ahhh. OK, I was unsure of that part. I have created a single patch and put that up. Can attachments be deleted?
by , 10 years ago
Attachment: | LOSTexture.patch added |
---|
Patch to fix LOS segfault crash when added during game play.
follow-up: 21 comment:19 by , 10 years ago
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.
comment:20 by , 10 years ago
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
follow-up: 22 comment:21 by , 10 years ago
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...
comment:22 by , 10 years ago
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.
comment:23 by , 10 years ago
Keywords: | LOS, review patch → LOS review patch |
---|
The patch fixes the bug for me on Windows 7, just needs a code review now.
comment:25 by , 10 years ago
Ok, so for a really late code review:
- what scythetwirler said
- your
wasShaderInit
name is quite unclear and needs am_
because it's a member variable. I'd personally call itm_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
comment:27 by , 10 years ago
Keywords: | LOS review patch removed |
---|
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! :)
comment:28 by , 9 years ago
No problem! I haven't been able to commit but am looking forward to working more on it when time frees up!
Do you have GLSL enabled?