Skip to content
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

[RDY] Extend 'generic_factory' + aliases + use the factory to manage terrain objects #16368

Merged
merged 18 commits into from
Apr 28, 2016

Conversation

codemime
Copy link
Contributor

@codemime codemime commented Apr 24, 2016

Continues #15977.

  • The generic_factory class became responsible for routine id operations (converting, validating, caching (b393d7c), checking (2ae3eff)) and now supports aliases (0cb8184)
  • Thereby the id caching mechanism (introduced in [RDY] Ad hoc string identifiers for terrain + generic id caching (refactoring) #15977) became properly generic
  • The generic_factory doesn't expose the map object (a9a5db3) and doesn't construct a vector for iterating purposes. It returns the internal vector instead (be7deb4). Should be faster.
  • The terrain objects (ter_t) are now managed using the factory (bea6b17). This means they can be properly modded.
  • Hard-coded terrain conversion table was dropped and replaced with aliases in JSON (c998cbd).

WIP because ter_t::load() doesn't use generic_factory loading features yet. Also, I'm going to refactor ter_t checking to make it use it's own function ter_t::check(). It's done.

static const T dummy{};
return dummy;
const T &obj( const string_id<T> &id ) const {
static int_id<T> i_id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this static?

Copy link
Contributor Author

@codemime codemime Apr 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent constructing the object on each obj() invocation. It gets reassigned anyway (x2).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's confusing and error prone. Constructing (and initializing) a single int (which is all that int_id contains) shouldn't take any time.

I actually suspect this will prevent the compiler from certain optimizations: a non-static variable (especially since this is effectively only an `int) can be put into a register. The static variable must be in main memory and may need to be loaded again and again.

Also: the compiler may see that id is immediately (and unconditionally) overwritten in find and may skip the initialization.

In short: looks like premature optimization.

@codemime codemime changed the title [WIP][CR] Extend 'generic_factory' + aliases + use the factory to manage terrain objects [RDY] Extend 'generic_factory' + aliases + use the factory to manage terrain objects Apr 25, 2016
@codemime
Copy link
Contributor Author

All set.

@codemime codemime force-pushed the extend_generic_factory branch from d66e065 to 7b31689 Compare April 25, 2016 11:14
@Coolthulhu Coolthulhu self-assigned this Apr 28, 2016
@Coolthulhu
Copy link
Contributor

"No Fungal Mod" overrides a terrain type. Before, it would silently do it, now it complains about duplicates.

@codemime
Copy link
Contributor Author

codemime commented Apr 28, 2016

It should. Terrain objects probably don't have "edit-mode" : "override" fields there.

@Coolthulhu
Copy link
Contributor

No fungal mod is bundled as official mod, it needs to be handled correctly.

@codemime
Copy link
Contributor Author

I found two terrain objects there. Fix them here or in a different PR?

@Coolthulhu
Copy link
Contributor

Here

@codemime
Copy link
Contributor Author

OK. Give me a couple of minutes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants