Page MenuHomeWildfire Games

Do not hardcode attacktypes in the engine/Atlas
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bb created this revision.Sep 5 2020, 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.Sep 5 2020, 10:54 PM
Stan added a comment.Sep 5 2020, 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
413 ↗(On Diff #13423)

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
408 ↗(On Diff #13423)
else if (boost::algorithm::starts_with(anim, "attack_") {
    // ...
}
412 ↗(On Diff #13423)

const std::string&.

414 ↗(On Diff #13423)

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

bb updated this revision to Diff 13425.Sep 6 2020, 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.Sep 6 2020, 12:16 AM
source/tools/atlas/GameInterface/ActorViewer.cpp
411 ↗(On Diff #13425)

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

423 ↗(On Diff #13425)

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

Silier resigned from this revision.Sep 6 2020, 10:01 AM
bb updated this revision to Diff 13431.Sep 6 2020, 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.Sep 6 2020, 12:39 PM
source/simulation2/components/ICmpAttack.cpp
39 ↗(On Diff #13431)

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.Sep 6 2020, 1:02 PM
Vulcan added a comment.Sep 6 2020, 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.Sep 7 2020, 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.

vladislavbelov accepted this revision.Oct 8 2020, 11:01 PM

I think we need unify and cleanup CStr/CStr/std::string/std::wstring things in future. That patch looks good, and it's definitely good for modders.

This revision is now accepted and ready to land.Oct 8 2020, 11:01 PM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Oct 10 2020, 5:14 PM