Page MenuHomeWildfire Games

Do not hardcode attacktypes in the engine/Atlas
Needs ReviewPublic

Authored by bb on Sat, Sep 5, 10:45 PM.

Details

Reviewers
Stan
Angen
Summary

The cpp part of D368, which is mostly unrelated to the patch and fixes rP23592

Test Plan

See that effectively nothing changed

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
SeverityLocationCodeMessage
Errorsource/tools/atlas/GameInterface/MessagesSetup.h:211CPPCheckBear (syntaxError)CPPCheckBear (syntaxError)
Unit
No Unit Test Coverage
Build Status
Buildable 13124
Build 26057: Vulcan BuildJenkins
Build 26056: Vulcan Build (macOS)Jenkins
Build 26055: Vulcan Build (Windows)Jenkins
Build 26054: arc lint + arc unit

Event Timeline

bb created this revision.Sat, Sep 5, 10:45 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1499/display/redirect

bb requested review of this revision.Sat, Sep 5, 10:54 PM
Stan added a comment.Sat, Sep 5, 11:13 PM

Wonder if we shouldn't use CStr but likely out of the scope of that patch.

source/tools/atlas/GameInterface/ActorViewer.cpp
411

Look into ICU provided functions which I believe are better than boost. Anyone can contradict me on this.

See the comment about boost likely using icu there
https://stackoverflow.com/questions/313970/how-to-convert-stdstring-to-lower-case/313990#313990Else you can use https://notfaq.wordpress.com/2007/08/04/cc-convert-string-to-upperlower-case/

Which uses std::

vladislavbelov added inline comments.
source/tools/atlas/GameInterface/ActorViewer.cpp
406–416
else if (boost::algorithm::starts_with(anim, "attack_") {
    // ...
}
410

const std::string&.

412

I'd suggest to name it repeatTime. And break after that.

bb updated this revision to Diff 13425.Sun, Sep 6, 12:01 AM
bb marked 2 inline comments as done.

Use Cstr, nuke boost

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1500/display/redirect

vladislavbelov added inline comments.Sun, Sep 6, 12:16 AM
source/tools/atlas/GameInterface/ActorViewer.cpp
411

Btw, we can check only suffixes here without string concatenation, but I don't mind here.

423–424

I think it should be if (repeatTime > 0.0f), since repeatTime is float (I suppose we don't use negative repeatTime).

Angen resigned from this revision.Sun, Sep 6, 10:01 AM
bb updated this revision to Diff 13431.Sun, Sep 6, 12:00 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1506/display/redirect

vladislavbelov added inline comments.Sun, Sep 6, 12:39 PM
source/simulation2/components/ICmpAttack.cpp
39

If use std::string above, maybe we need to use std::string here as well?

I found usages of CStrW, CStr and std::string as types in components interfaces (std::vector only of std::string).

bb updated this revision to Diff 13434.Sun, Sep 6, 1:02 PM
Vulcan added a comment.Sun, Sep 6, 1:08 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1509/display/redirect

wraitii added a subscriber: wraitii.Mon, Sep 7, 9:55 AM

For what it's worth, javascript uses UTF-16 strings, so it's probably faster to use CStrW when interacting with JS code (fewer conversions). That being said, performance is not the utmost concern here, so using utf-8 strings is probably neater.

The patch looks good to me, but I haven't tested it.

For what it's worth, javascript uses UTF-16 strings, so it's probably faster to use CStrW when interacting with JS code (fewer conversions). That being said, performance is not the utmost concern here, so using utf-8 strings is probably neater.

@linkmauve had a suggestion to use only UTF-8 on the C++ side to remove the hell with CStrW and its definition.