Skip to content

Conversation

@mateofio
Copy link
Contributor

@mateofio mateofio commented Aug 19, 2020

Depends on: EasyRPG/liblcf#384

Fix: #2281

This is a new interface to access the lcf database and treemap from all Player code.

Essentially we will change from this:

auto& actors = lcf::Data::actors;
auto* actor = lcf::ReaderUtil::GetElement(actors, 1);

To this

auto& actors = db::Actors();

// Assumed id is always valid, asserts in debug build
auto& actor = db::Get(actors, 1);

// Requires you to check the pointer for validity
auto* actor = db::GetPtr(actors, 1);

// Like before, but also logs a warning if id is invalid
auto* actor = db::GetPtr(actors, 1, "MyFunction");

The reasons for changing this interface are:

  • The database singleton is now part of player code and not lcf
  • Enables us refactor player before removing lcf::Data and all singletons from liblcf.
  • More concise name db::
  • Potentially faster compile speeds. Including db.h doesn't bring in any lcf/rpg headers, only forward declarations.

I'm posting this for review on the interface. Once everyone is on board with the design, I'll add commits to this porting Player code to use the new interface. Need to avoid rebase hell on this one.

@mateofio mateofio marked this pull request as draft August 19, 2020 22:12
@fdelapena fdelapena added Performance Related to improvements on memory, less CPU or disk usage, battery savings, etc. Has PR Dependencies This PR depends on another PR Refactor labels Aug 20, 2020
@fdelapena fdelapena removed the Has PR Dependencies This PR depends on another PR label Aug 24, 2020
@mateofio mateofio force-pushed the db branch 2 times, most recently from 5506151 to 50e098d Compare August 25, 2020 17:45
@Ghabry
Copy link
Member

Ghabry commented Oct 5, 2020

Imo this looks fine. The Editor code is also already written in such a way that there is no global access anymore. What I would suggest here:

  • Merge all other PRs first (obviously) ;)
  • Convert one class (e.g. Actors) first as a "playground"
  • When the interface is decided after playing around change the rest
  • Remove lcf global instances (needs changes to LDB/LMT loading)

@fdelapena fdelapena added Has PR Dependencies This PR depends on another PR and removed Has PR Dependencies This PR depends on another PR labels Oct 6, 2020
@fdelapena fdelapena added the Awaiting Rebase Pull requests with conflicting files due to former merge label May 28, 2022
@Ghabry
Copy link
Member

Ghabry commented Nov 2, 2023

Closing this due to lack of interest and progress. Please reopen when there are any updates.

@Ghabry Ghabry closed this Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Awaiting Rebase Pull requests with conflicting files due to former merge Performance Related to improvements on memory, less CPU or disk usage, battery savings, etc. Refactor

Development

Successfully merging this pull request may close these issues.

Provide a new singleton abstraction for liblcf ldb,lmt,lmu data

3 participants