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

Refactor fields (step 6) #32481

Merged
merged 7 commits into from
Jul 23, 2019

Conversation

ZhilkinSerg
Copy link
Contributor

@ZhilkinSerg ZhilkinSerg commented Jul 18, 2019

Summary

SUMMARY: Infrastructure "Refactor fields (step 6)"

Purpose of change

Unhardcode data structures to allow field type customization/addition via json.

Describe the solution

Current pull request does the following:

  • minor typo fix;
  • unhardcodes several field type attributes (decay amount factor, apply slime factor, transparency cache invalidation, light emitted, field_highlight on character and translucency).

@ZhilkinSerg ZhilkinSerg added [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. labels Jul 18, 2019
bool has_fume = false;
for( const auto &field : fields ) {
if( field.first == fd_incendiary || field.first == fd_fire ) {
has_fire = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add fd_fire_vent and fd_flame_burst fields too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was fixed in following linked PR.

intensity_levels.emplace_back( intensity_level );
}
optional( jo, was_loaded, "underwater_age_speedup", underwater_age_speedup, 0_turns );
optional( jo, was_loaded, "decay_amount_factor", decay_amount_factor, 0 );
optional( jo, was_loaded, "apply_slime_factor", apply_slime_factor, 0 );
optional( jo, was_loaded, "dirty_transparency_cache", dirty_transparency_cache, false );
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that this dirty_transparency_cache bool may be able to be determined automatically at runtime based on whether the field emits light or is translucent. Determining it automatically would imo be more robust than requiring people modifying/adding new fields to set it correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. That is not final state of field type object and I plan to revisit it later. Right now it is just easier to keep flags for all field types in one place in explicit form even if they are somewhat redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, this jsonization is a pretty massive undertaking already and I'm totally on board with keeping the code simpler for now.

@@ -413,6 +413,8 @@ bool map::process_fields_in_submap( submap *const current_submap,
debugmsg( "Whoooooa intensity of %d", cur.get_field_intensity() );
}

dirty_transparency_cache = curtype.obj().dirty_transparency_cache;
Copy link
Member

Choose a reason for hiding this comment

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

The logic here is wrong, it needs to "latch" to dirty. Right now it will be un-set when it processes an invisible field.

Suggested change
dirty_transparency_cache = curtype.obj().dirty_transparency_cache;
dirty_transparency_cache |= curtype.obj().dirty_transparency_cache;

I think this will do it, however when I test the smoke from a smoke grenade still isn't acting non-transparent.

Copy link
Member

Choose a reason for hiding this comment

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

Re-tested with current master and it's already broken, so that's great :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe be I broke it earlier? Need to test in 0.D...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0.D is working, so I need to bisect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Smoke is non-transparent starting from intensity level 2.

Maybe it is not transparency, but field intensity issue (i.e. smoke from smoke bombs aren't achieving level 2 or 3 anymore)?

I've tested on savegame from 0.D with exploded smoke bomb and there is thick smoke in the center which is non-transparent, but newly exploded smoke bomb only gives intensity level 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in #32551.

@kevingranade kevingranade merged commit 9dd7b1e into CleverRaven:master Jul 23, 2019
@ZhilkinSerg ZhilkinSerg deleted the refactor-fields-step-6 branch July 23, 2019 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants