HomeWildfire Games

List, add and delete cinematic paths in Atlas.
AuditedrP19427

Description

List, add and delete cinematic paths in Atlas.
Doesn't provide a way to add/delete nodes of paths yet.

Differential Revision: https://code.wildfiregames.com/D348
Patch By: Vladislav

Event Timeline

Itms raised a concern with this commit.Apr 30 2017, 1:35 PM
Itms added a subscriber: Itms.

This commit introduced the use of wxString::ToStdWstring which was introduced in wxWidgets 3. This effectively breaks compatibility with wxWidgets 2.8. ?

We could of course look into dropping 2.8, but it would make much more sense to change this and convert this string the same way we convert all other strings, in a legacy fashion.

/ps/trunk/source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Cinema/Cinema.cpp
98

This is the faulty change.

113

This is the faulty change.

This commit now has outstanding concerns.Apr 30 2017, 1:35 PM
vladislavbelov accepted this commit.May 1 2017, 3:16 AM
vladislavbelov added a subscriber: vladislavbelov.
In rP19427#20634, @Itms wrote:

This commit introduced the use of wxString::ToStdWstring which was introduced in wxWidgets 3. This effectively breaks compatibility with wxWidgets 2.8. ?

We could of course look into dropping 2.8, but it would make much more sense to change this and convert this string the same way we convert all other strings, in a legacy fashion.

Since we have no strict rules about wxWidget (https://trac.wildfiregames.com/wiki/BuildInstructions?action=history), it's not the issue.

Since we have no strict rules about wxWidget (https://trac.wildfiregames.com/wiki/BuildInstructions?action=history), it's not the issue.

Not explicitly recommending old versions (2.8) is not the same thing as silently breaking support for it.
As long as providing support for 2.8 is relatively trivial, I don't see why we should break that.

vladislavbelov added a comment.EditedMay 1 2017, 3:32 AM

Since we have no strict rules about wxWidget (https://trac.wildfiregames.com/wiki/BuildInstructions?action=history), it's not the issue.

Not explicitly recommending old versions (2.8) is not the same thing as silently breaking support for it.
As long as providing support for 2.8 is relatively trivial, I don't see why we should break that.

Because I don't want to look a the dependencies for each OS each time, when I'm writing a code. So we fix the lib version or/and we fix how we use the lib.

elexis added a comment.May 1 2017, 4:06 AM

Since we have no strict rules about wxWidget (https://trac.wildfiregames.com/wiki/BuildInstructions?action=history), it's not the issue.

Not explicitly recommending old versions (2.8) is not the same thing as silently breaking support for it.

(for Mandriva it is recommended (required?) even)

Since we have no strict rules about wxWidget (https://trac.wildfiregames.com/wiki/BuildInstructions?action=history), it's not the issue.

Not explicitly recommending old versions (2.8) is not the same thing as silently breaking support for it.

(for Mandriva it is recommended (required?) even)

Per distro commands/hints are user provided, outdated, and should be taken with about an ocean of salt.

But trying to remove those just results in people adding them back or crying about how reading and figuring out what packages are needed is hard.

That said if someone wants to improve the commands, it is a wiki.

Itms added a comment.May 1 2017, 11:05 AM

Because I don't want to look a the dependencies for each OS each time, when I'm writing a code. So we fix the lib version or/and we fix how we use the lib.

Well by writing code that uses wxWidgets, you are responsible for breaking changes like this one, so you should ideally take a look at the dependencies... But if you miss something, that's not a big deal: someone else will notice during review or through an Audit ?

@leper: To settle the wiki page issue, would it be OK to write

Our code should stay compatible with wxWidgets 2.8 as long as we can, but upgrading to 3.0 is recommended if your distribution offers it.

?

@leper: To settle the wiki page issue, would it be OK to write

Our code should stay compatible with wxWidgets 2.8 as long as we can, but upgrading to 3.0 is recommended if your distribution offers it.

?

I don't think the https://trac.wildfiregames.com/wiki/BuildInstructions page should be concerned with what the code should do (staying compatible).

We might want to add a comment about (at least wxWidgets version X.Y.Z) to https://trac.wildfiregames.com/wiki/BuildInstructions#Dependencies, which we might want to unify for *nix (or *nix without OS X, or macOS, or Mac OS X, or the operating system formerly known as NeXTSTEP).

That said I'd add some warnings about those specific install commands most likely being outdated and that people should look at that dependency list instead. (Or just remove those commands and tell users to RTFM.)

Itms accepted this commit.May 7 2017, 5:51 PM

D427 fixed the issue. Thanks ?

All concerns with this commit have now been addressed.May 7 2017, 5:51 PM
elexis added a comment.May 7 2017, 6:49 PM

For the proposed wiki page that could be linked to both pages, see #4551