New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimized and correct Flags #237

Merged
merged 4 commits into from Oct 4, 2018

Conversation

Projects
None yet
4 participants
@fmatthew5876
Contributor

fmatthew5876 commented Sep 27, 2018

  • Add is2k3 field to flags.csv
  • Support writing different 2k vs 2k3 size
    (example: trooppage condition flags)
  • Correctly writes empty flags to ldb file
  • No more dynamically allocated objects
  • Setup union for easy array access of flags in rpg headers
  • Binary search static array of tag strings, no std::map.
  • Optimzies read/write routines

@fmatthew5876 fmatthew5876 changed the title from Enhance Flags write support for ldb to Optimized and correct Flags Sep 27, 2018

#include "rpg_animationcelldata.h"
#include <array>

This comment has been minimized.

@Ghabry

Ghabry Sep 27, 2018

Member

too many includes, iirc you have to handle thsi in "struct_headers" function

@@ -595,9 +595,10 @@ template <class S>
class Flags {
public:
struct Flag {
Flag(bool S::*ref, const char* const name) : ref(ref), name(name) {}
Flag(bool S::*ref, const char* const name, bool is2k2) : ref(ref), name(name), is2k3(is2k3) {}

This comment has been minimized.

@Ghabry

Ghabry Sep 27, 2018

Member

is2k2

@Ghabry

This comment has been minimized.

Member

Ghabry commented Sep 27, 2018

Jenkins: Test this please <3

Nice changes, also looks harmless concerning pre-0.5.4 merging 👍

Edit: Compile failure, pls see our jenkins output :)

@fdelapena

This comment has been minimized.

Contributor

fdelapena commented Sep 27, 2018

Jenkins: ok to test

@fmatthew5876

This comment has been minimized.

Contributor

fmatthew5876 commented Sep 27, 2018

Fixed the issues mentioned. I also changed the anonymous union / struct thing to build under older compilers by writing a constructor. It's uglier and would be good to eliminate when we can.

Can you verify I got the is2k3 flags right for everything in flags.csv?

@Ghabry

Ghabry approved these changes Sep 27, 2018

@Ghabry

This comment has been minimized.

Member

Ghabry commented Sep 27, 2018

The flags look correct for me. SavePicture is a special case: This is a new feature of RPG Maker 2003 (English release) but there is no way to detect this version inside the database alone.
Though I don't think that marking it as 2k3 will have negative side effects.

@fmatthew5876

This comment has been minimized.

Contributor

fmatthew5876 commented Sep 27, 2018

Rebased onto style fixed actor changes.

@fdelapena

This comment has been minimized.

Contributor

fdelapena commented Sep 27, 2018

Thanks!

We've added you to the Player Team, this should whitelist automatic pull request building in Jenkins CI for multiple EasyRPG repositories. At your option, you may keep the team private if you don't want to display it in your GitHub Profile.

@fmatthew5876

This comment has been minimized.

Contributor

fmatthew5876 commented Sep 27, 2018

I don't know how I got it into my head that the flag names were sorted.

I've removed the binary search in favor of a simple linear search. This is still faster than std::map because N is small. map has really bad locality and wastes a lot of memory storing pointers. It's also much simpler.

@fmatthew5876

This comment has been minimized.

Contributor

fmatthew5876 commented Sep 30, 2018

Rebased onto master

@Ghabry

Ghabry approved these changes Sep 30, 2018

{%- for flag in flags[struct_name] %}
bool {{ flag.field }};
{%- endfor %}
};

This comment has been minimized.

@carstene1ns

carstene1ns Sep 30, 2018

Member

gcc dislikes these anonymous structs, btw.
warning: ISO C++ prohibits anonymous structs

https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html

@fmatthew5876

This comment has been minimized.

Contributor

fmatthew5876 commented Sep 30, 2018

I pushed a new commit to disable gcc warnings about C11 anonymous structs.

@Ghabry

This comment has been minimized.

Member

Ghabry commented Oct 2, 2018

The problem is now that these pragmas make Visual Studio / MSVC extremely spammy with "unknown pragma" warnings (1000s of warnings)

So the pragma must be guarded with "#ifndef _MSC_VER" ... 😥

@fmatthew5876

This comment has been minimized.

Contributor

fmatthew5876 commented Oct 2, 2018

Maybe we can just not compile with -pedantic with autotools? -Wall -Wextra should be enough.

EDIT: I don't even see any -pedantic in the repository.

Maybe we just need to change the jenkins script:

[liblcf-linux] $ /bin/sh -xe /tmp/jenkins822786638251706731.sh
+ export STATICLIBSPATH=/var/lib/jenkins/workspace/toolchain-linux-static/linux-static
+ STATICLIBSPATH=/var/lib/jenkins/workspace/toolchain-linux-static/linux-static
+ export PKG_CONFIG_PATH=/var/lib/jenkins/workspace/toolchain-linux-static/linux-static/lib/pkgconfig
+ PKG_CONFIG_PATH=/var/lib/jenkins/workspace/toolchain-linux-static/linux-static/lib/pkgconfig
+ export 'CXXFLAGS=-Wall -Wextra -pedantic -O0 -g3'
+ CXXFLAGS='-Wall -Wextra -pedantic -O0 -g3'
+ autoreconf -fi

@fdelapena fdelapena added this to the 0.5.4 milestone Oct 2, 2018

@Ghabry

This comment has been minimized.

Member

Ghabry commented Oct 3, 2018

hm yeah, I would slowly prefer removing -pedantic from the jenkins job ;)

@fmatthew5876

This comment has been minimized.

Contributor

fmatthew5876 commented Oct 3, 2018

Well I put the ugly guards in. So let me know what you decide. I can remove them or leave them there.

fmatthew5876 added some commits Sep 27, 2018

Enhance Flags write support for ldb
- Add is2k3 field to flags.csv
- Support writing different 2k vs 2k3 size
    (example: trooppage condition flags)
- Correctly writes empty flags to ldb file
Optimize flags and flags parser
- No more dynamically allocated objects
- Setup union for easy array access of flags in rpg headers
- Linear search static array of tag strings, no std::map.
- Optimzies read/write routines
@fmatthew5876

This comment has been minimized.

Contributor

fmatthew5876 commented Oct 4, 2018

Rebased onto master

Ignore parameters of Flags' LcfSize()
These are not used anymore.

@carstene1ns carstene1ns merged commit 71a90e1 into EasyRPG:master Oct 4, 2018

5 checks passed

GCW0 Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows Build finished.
Details
web Build finished.
Details

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:ldb_flags branch Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment