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

Fix Tiles Regression from Room Transition #1349

Merged
merged 4 commits into from Aug 5, 2018

Conversation

Projects
None yet
3 participants
@RobertBColton
Member

RobertBColton commented Aug 5, 2018

This fixes the tiles in HitCoder's Sonic game as we discussed over Discord.

HitCoder's Sonic ENIGMA Game

Basically, polygonz's delete_tiles function is already called on room transition. It doesn't need to delete the vertex/index buffers we lazily create for drawing tiles because other rooms may draw tiles later or the user may transition back to a room with tiles. We do however still need to make them dirty so that they are cleared on the next frame (aka screen_redraw call).

Summary of changes:

  • Moved bkinxcomp and tiles_are_dirty to an anonymous namespace since they are not used outside of the tiles compilation unit.
  • Created the user-space functions vertex_exists and index_exists to check if a vertex/index buffer exists.
  • Replaced the internal comparisons to -1 of the tile vertex/index buffers with the exists functions.
  • Added a comment explaining why I kept the signature for rebuild_tile_layer and what the later plans for that function are.
  • Changed delete_tiles to simply mark the tiles as dirty and needing an update on the next frame.
  • Added a room transition test that reproduces HitCoder's issue in the form of a GMX project.
@codecov

This comment has been minimized.

codecov bot commented Aug 5, 2018

Codecov Report

Merging #1349 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1349      +/-   ##
==========================================
+ Coverage   13.46%   13.62%   +0.15%     
==========================================
  Files         168      168              
  Lines       17428    17431       +3     
==========================================
+ Hits         2347     2375      +28     
+ Misses      15081    15056      -25
Impacted Files Coverage Δ
...system/SHELL/Graphics_Systems/General/GSvertex.cpp 42.78% <100%> (+2.25%) ⬆️
...Asystem/SHELL/Graphics_Systems/General/GStiles.cpp 40.5% <100%> (-0.17%) ⬇️
...Asystem/SHELL/Universal_System/instance_system.cpp 51.59% <0%> (+0.53%) ⬆️
ENIGMAsystem/SHELL/Universal_System/roomsystem.cpp 22.71% <0%> (+3.1%) ⬆️
ENIGMAsystem/SHELL/Universal_System/instance.cpp 10.16% <0%> (+10.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88383a3...0e1766b. Read the comment docs.

@RobertBColton RobertBColton force-pushed the tiles-room-goto-fix branch from a326ed7 to fdc2de3 Aug 5, 2018

@RobertBColton RobertBColton force-pushed the tiles-room-goto-fix branch from fdc2de3 to 003a525 Aug 5, 2018

@EnigmaBot

This comment has been minimized.

EnigmaBot commented Aug 5, 2018

Warning: The following images are found in the pull request but not master (new tests?):

enigma_tiles_room_transition_test.png

enigma_tiles_room_transition_test.png

@@ -0,0 +1,7 @@
{\rtf1\ansi

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 5, 2018

Member

You can probably delete this file and the icon without damaging anything. At least, I'd hope you can.

@@ -0,0 +1,44 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 5, 2018

Member

This file, I don't know about. This is all default, yes? Is it necessary?

@EnigmaBot

This comment has been minimized.

EnigmaBot commented Aug 5, 2018

Warning: The following images are found in the pull request but not master (new tests?):

enigma_tiles_room_transition_test.png

enigma_tiles_room_transition_test.png

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 5, 2018

@EnigmaBot

This comment has been minimized.

EnigmaBot commented Aug 5, 2018

Warning: The following images are found in the pull request but not master (new tests?):

enigma_tiles_room_transition_test.png

enigma_tiles_room_transition_test.png

@RobertBColton RobertBColton merged commit 0b9cfb5 into master Aug 5, 2018

4 checks passed

codecov/patch 100% of diff hit (target 13.46%)
Details
codecov/project 13.62% (+0.15%) compared to 88383a3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RobertBColton RobertBColton deleted the tiles-room-goto-fix branch Aug 5, 2018

JoshDreamland added a commit that referenced this pull request Nov 10, 2018

Fix Tiles Regression from Room Transition (#1349)
* Moved `bkinxcomp` and `tiles_are_dirty` to an anonymous namespace since they are not used outside of the tiles compilation unit.
* Created the user-space functions `vertex_exists` and `index_exists` to check if a vertex/index buffer exists.
* Replaced the internal comparisons to `-1` of the tile vertex/index buffers with the exists functions.
* Added a comment explaining why I kept the signature for `rebuild_tile_layer` and what the later plans for that function are.
* Changed `delete_tiles` to simply mark the tiles as dirty and needing an update on the next frame.
* Added a room transition test that reproduces HitCoder's issue in the form of a GMX project.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment