Paths

Table of Contentst

Diffusion 0 A.D. Public Repository rP19317

Allow Cinematic Path nodes that only specify a camera position or direction and…
AuditedrP19317
Actions

Description

Allow Cinematic Path nodes that only specify a camera position or direction and group them by deltatime.
Remove the support of saving the rotation of camera position nodes, because that should be rewritten to use a custom spline.

Patch By: Vladislav
Differential Revision: https://code.wildfiregames.com/D124
Refs #3814

Details

Auditors

vladislavbelov
• leper
Sandarac

Committed

elexis

Mar 20 2017, 4:26 PM

Differential Revision

D124: Fixes the reading/writing of cinematic paths

Parents

rP19316: Catafalque wagon unit

Branches

Unknown

Tags

Unknown

Build Status

Buildable 819
Build 1289: Post-Commit Build	Jenkins

Event Timeline

elexis committed rP19317: Allow Cinematic Path nodes that only specify a camera position or direction and….Mar 20 2017, 4:26 PM

Harbormaster completed building B819: rP19317: Allow Cinematic Path nodes that only specify a camera position or direction and….Mar 20 2017, 5:11 PM

Reported by Hannibal Barca on open suse:

MapWriter.cpp
In file included from /usr/include/c++/4.8/algorithm:62:0,
                 from ../../../source/lib/lib.h:66,
                 from ../../../source/lib/precompiled.h:72,
                 from ../../../source/pch/graphics/precompiled.h:18:
/usr/include/c++/4.8/bits/stl_algo.h: In instantiation of ‘_RandomAccessIterator std::__unguarded_partition(_RandomAccessIterator, _RandomAccessIterator, const _Tp&) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent*, std::vector<CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent> >; _Tp = CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent]’:
/usr/include/c++/4.8/bits/stl_algo.h:2283:70:   required from ‘_RandomAccessIterator std::__unguarded_partition_pivot(_RandomAccessIterator, _RandomAccessIterator) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent*, std::vector<CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent> >]’
/usr/include/c++/4.8/bits/stl_algo.h:2315:54:   required from ‘void std::__introsort_loop(_RandomAccessIterator, _RandomAccessIterator, _Size) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent*, std::vector<CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent> >; _Size = long int]’
/usr/include/c++/4.8/bits/stl_algo.h:5461:36:   required from ‘void std::sort(_RAIter, _RAIter) [with _RAIter = __gnu_cxx::__normal_iterator<CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent*, std::vector<CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent> >]’
../../../source/graphics/MapWriter.cpp:456:43:   required from here
/usr/include/c++/4.8/bits/stl_algo.h:2245:19: error: passing ‘const CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent’ as ‘this’ argument of ‘bool CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent::operator<(const CMapWriter::WriteXML(const VfsPath&, WaterManager*, SkyManager*, CLightEnv*, CCamera*, CCinemaManager*, CPostprocManager*, CSimulation2*)::SEvent&)’ discards qualifiers [-fpermissive]
    while (__pivot < *__last)
                   ^
graphics.make:317: recipe for target 'obj/graphics_Release/MapWriter.o' failed
make[1]: *** [obj/graphics_Release/MapWriter.o] Error 1
Makefile:48: recipe for target 'graphics' failed
make: *** [graphics] Error 2

Reproduced by Vladislav here https://godbolt.org/g/SoZoj5
Seems we need C++ 11 to build that part of the code.

Refs not being able to raise a concern on ones own commit

Sandarac raised a concern with this commit.Mar 22 2017, 2:04 AM

This commit now has outstanding concerns.Mar 22 2017, 2:04 AM

Er, isn't that compiler below the minimum required version? C++11 is required for the game. I'm not sure if there is an issue with this commit.

In rP19317#19923, @Sandarac wrote:

Er, isn't that compiler below the minimum required version? C++11 is required for the game. I'm not sure if there is an issue with this commit.

Hm, you're right: http://trac.wildfiregames.com/wiki/BuildInstructions
People should still be able to compile the game, but I guess that is to be done by updating their system.

This commit now requires verification by auditors.Mar 22 2017, 1:18 PM

vladislavbelov accepted this commit.Mar 22 2017, 5:51 PM

(17:50:08) elexis: Hannibal_Baraq: export CC=gcc-6 CXX=g++-6
(17:51:43) Hannibal_Baraq: works
(17:51:44) Hannibal_Baraq: thx

What's the output of gcc --version? If that is something that is supported, what is the version of stdlibc++?

That said the error message complains about something const being passed to something non-const and discarding qualifiers, so addressing the const comment should take care of that.

/ps/trunk/source/graphics/MapReader.cpp
892	This looks a lot like it could be done by initializing those using a ctor.
/ps/trunk/source/graphics/MapWriter.cpp
431	Why isn't this const? That looks a lot like the reason for the linking failure.
438	Might be nice to reserve the proper amount as we do know it at this point already.

This commit now has outstanding concerns.Mar 22 2017, 9:48 PM

vladislavbelov added a subscriber: vladislavbelov.Mar 23 2017, 12:21 AM

vladislavbelov added inline comments.

/ps/trunk/source/graphics/MapReader.cpp
892	Nope, it couldn't, it gives a compilation error.
/ps/trunk/source/graphics/MapWriter.cpp
431	Probably, it gives the failure, but from cppref: "signature does not need to have const &, but the function object must not modify the objects passed to it.".
438	There isn't any bottleneck, the vector is small enough to reserve it.

vladislavbelov added inline comments.Mar 23 2017, 12:23 AM

/ps/trunk/source/graphics/MapReader.cpp
892	But if you mean the vector, then the current way has more readability.

• leper added inline comments.Mar 23 2017, 12:26 AM

/ps/trunk/source/graphics/MapReader.cpp
892	Why would `CFixedVector3D position(fixed::FromString(attrs.GetNamedItem(at_x)), fixed::FromString(attrs.GetNamedItem(at_y)), fixed::FromString(attrs.GetNamedItem(at_z)));` do that?
/ps/trunk/source/graphics/MapWriter.cpp
431	But why not simply make it const and fix that error while you're at it?
438	And possibly cause a pointless reallocation or two depending on what people do with paths later on...

vladislavbelov added inline comments.Mar 23 2017, 12:37 AM

/ps/trunk/source/graphics/MapReader.cpp
892	Then it's not the one place, amount of this kinds of init, I'm not inviting something new, just use the other source to produces the same code-style.
/ps/trunk/source/graphics/MapWriter.cpp
431	Not C++11 compiler and more important things in other source.
438	I think somethinkg like log(N) reallocations in average.

• leper added inline comments.Mar 23 2017, 12:43 AM

/ps/trunk/source/graphics/MapReader.cpp
892	Ah, well then maybe do that in some cleanup commit for all those occurences at some point (you might even get away with adding a helper just for that).
/ps/trunk/source/graphics/MapWriter.cpp
431	The operator can and should still be const, and judging from the error message that would even fix that possibly slightly unsupported error while you're at it. And yes there are other important things that need to be done, but trying hard not to do them will just result in them not getting fixed. (Also doesn't help with motivating people to review your code if you apparently don't consider fixing issues in that important.)
438	Or none, if you just call `reserve` before the vector has to allocate memory to store anything, and the right size at that. Just because possibly fragmenting memory is easy doesn't mean that we should try hard to do that.

vladislavbelov added inline comments.Mar 23 2017, 12:44 AM

/ps/trunk/source/graphics/MapWriter.cpp
431	If add const, then I get an error: "error C2512: 'CMapWriter::WriteXML::SEvent' : no appropriate default constructor available C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\xmemory0". "

• leper added inline comments.Mar 23 2017, 12:46 AM

/ps/trunk/source/graphics/MapWriter.cpp
431	For `bool operator<(const SEvent& another) const`?

vladislavbelov added inline comments.Mar 23 2017, 12:48 AM

/ps/trunk/source/graphics/MapWriter.cpp
431	Ah, no, ok, it's all reserve, it wants a default ctor.

Fixes in the D250.

elexis mentioned this in D250: Fix legacy compilers build failure with cinematic path improvements in 19317.Mar 23 2017, 2:51 AM

Installed package apparently had been gcc-4.8-9.61 yesterday before he managed to succeed with gcc6

In rP19317#19948, @leper wrote:

What's the output of gcc --version?

Today he answered this question with 4.8.5

If that is something that is supported, what is the version of stdlibc++?

Going to be answered soon

Anyway, I guess older compilers will be supported regardless

If that is something that is supported, what is the version of stdlibc++?

Going to be answered soon

Anyway, I guess older compilers will be supported regardless

4.8.5 is supported. That said if the issue is something about as easy as this (and my guess at the fix is correct) I see no issue in fixing that, however I don't care about supporting non C++11 conformant compilers more than half a decade after that was finalized.

Fixed by setting gcc6 as default by following commands

sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-4.8 100 --slave /usr/bin/g++ g++ /usr/bin/g++-4.8
sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-6 100 --slave /usr/bin/g++ g++ /usr/bin/g++-6
sudo update-alternatives --config gcc

And selecting gcc6. Sorry for complication.

In rP19317#20050, @Hannibal_Barca wrote:

Fixed by setting gcc6 as default by following commands

sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-4.8 100 --slave /usr/bin/g++ g++ /usr/bin/g++-4.8
sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-6 100 --slave /usr/bin/g++ g++ /usr/bin/g++-6
sudo update-alternatives --config gcc

And selecting gcc6. Sorry for complication.

Please, reply to D250.

D250 was merged, requests for testing were ignored: worksforme.

Sandarac resigned from this commit.Mar 25 2017, 5:39 AM

All concerns with this commit have now been addressed.Mar 25 2017, 5:39 AM