Paths

Table of Contentst

FCollada: Various compilation fixes for different compilers
ClosedPublic
Actions

Authored by Stan on Dec 17 2018, 2:47 PM.

Details

Reviewers

Itms
historic_bruno

Commits

rP24322: Fix some compiler warnings. Use v141_xp toolset.

Trac Tickets

#4460

Summary

Add the fixes we don't have from here: https://github.com/rdb/fcollada/commit/f6186c5e9258106dbe82120a359c540876016f9e
Update the includes that didn't match the ones in the source folder.
Related: D1696, D1697

Test Plan

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 6509
Build 10766: Vulcan Build	Jenkins

Event Timeline

Stan created this revision.Dec 17 2018, 2:47 PM

Add missing parenthesis

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

Link to build: https://jenkins.wildfiregames.com/job/differential/825/

Harbormaster failed remote builds in B6509: Diff 7051!Dec 17 2018, 2:57 PM

Stan changed the repository for this revision from rP 0 A.D. Public Repository to Unknown Object (Repository).Dec 17 2018, 4:00 PM

Do we have to update our fcollada? Especially if it has useful changes.

If we have to, then my comments don't make sense. We just follow fcollada changes then.

include/FUtils/FUStringBuilder.h
139	Why `uint64`? The `size_t` isn't 64 bits always.
include/FUtils/FUTracker.h
119	I'd prefer to name a variable without leading underscores. This type of naming reserved for a global scope and for all scopes with a following capital letter.
122	This one and the same below fixes N6 from #5288.
include/FUtils/Platforms.h
128	No need to have a space after `defined`.

Updating Fcollada is not that important. Nevertheless though our debug build is broken on windows because of it and the game doesn't work with Fcollada compiled with vs2015 instant default.

So I thought we might as well take all the fixes from the other forks over the internet.

Fcollada is not being maintained anymore. I believe we are the only ones maintaining it

In D1698#72233, @Stan wrote:

Updating Fcollada is not that important. Nevertheless though our debug build is broken on windows because of it and the game doesn't work with Fcollada compiled with vs2015 instant default.

So I thought we might as well take all the fixes from the other forks over the internet.

Ok. some of my comments make sense then :)

Could you attach the errors?

By the way, the game is compiled for me without the patch with VS2013. Probably we need to test the patch for VS2013 too until we drop its support.

Errors for debug build are here https://trac.wildfiregames.com/ticket/4460
To reproduce just start a game in debug mode.

The Fcollada crash makes it impossible to package mods on windows.

For the crash after building Fcollada you just have to build it with vs2015. Vs 2013 works fine.

In D1698#72235, @Stan wrote:

Errors for debug build are here https://trac.wildfiregames.com/ticket/4460

Ok, it should be linked then (I added).

For the crash after building Fcollada you just have to build it with vs2015.
Vs 2013 works fine.

So you did a test of the patch on VS2013?

I can't test on VS2013 but the goal is to drop support eventually and move on to 2015 2017 and maybe 2019

In D1698#72240, @Stan wrote:

the goal is to drop support eventually and move on to 2015 2017

It's true, but we didn't drop it yet.

and maybe 2019

Does the VS2019 support Windows XP?

It's still using the 2017 toolset so yes.

In D1698#72244, @Stan wrote:

It's still using the 2017 toolset so yes.

Ok then. I'm waiting for answers of my comments. I agree with the patch in general.

Also it'd be good to test the patch on macOS and openBSD since the patches touches related macros.

historic_bruno added a subscriber: historic_bruno.Jul 19 2019, 8:16 AM

historic_bruno mentioned this in D1696: FCollada: Add gitignore and Update solution files.Jul 19 2019, 6:37 PM

historic_bruno mentioned this in D1697: FCollada: Add missing includes in the test suite.

historic_bruno retitled this revision from Various compilation fixes for different compilers to FCollada: Various compilation fixes for different compilers.

historic_bruno edited the summary of this revision. (Show Details)

historic_bruno added a reviewer: historic_bruno.Jul 20 2019, 7:10 PM

historic_bruno added inline comments.Jul 20 2019, 11:06 PM

include/FUtils/FUStringBuilder.h
139	Guess it's a hack for assumed 64-bit Macs and OpenBSD. At least it could use a `#warning TODO` to draw attention to it. (The header in FCollada source already has this change, this diff syncs them)
include/FUtils/FUTracker.h
119	That's FCollada conventions, they have some strange ones...
include/FUtils/Platforms.h
128	This was a mixup in the header syncing. In some cases the game's "include" folder are newer, and in some cases the FCollada source "includes" are newer. I have a new patch that resolves this.