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

CRYO: EDEN: Move graphics into a separate class #1323

Merged
merged 1 commit into from Sep 11, 2019

Conversation

@dafioram
Copy link
Member

commented Sep 1, 2018

Eden game object now contains a graphics object with which to
delegate graphics operations and store states of the graphics.

Before all the graphics and eden code where together.

Much of the video playing is done in the graphics class
so I have moved a lot of the video state into there.

Some graphics related variables were moved out of eden and into
graphics, but many are still in eden.

Since they are still coupled there are lots of getters and setters.
For example both eden_graphics and eden share a handle to the same
video object.

I have made a few more things public than desirable.

I changed graphics to eden_graphics since it is specialized to eden
and not just cryo.

CRYO: EDEN: Move graphics into a separate class
Eden game object now contains a graphics object with which to
delegate graphics operations and store states of the graphics.

Much of the video playing is done in the graphics class
so I have moved a lot of the video state into there.

Some graphics related variables were moved out of eden and into
graphics, but many are still in eden.

Since they are still coupled there are lots of getters and setters.
For example both eden_graphics and eden share a handle to the same
video object.

I have made a few more things public than desirable.

I changed graphics to eden_graphics since it is specialized to eden
and not just cryo.

@dafioram dafioram requested a review from Strangerke Sep 1, 2018

@dafioram dafioram closed this Oct 5, 2018

@sev-

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Why did you close it? The changes are fine, and they're worth merging.

@dafioram

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

It's very possible that bugs have been introduced in this partial refactoring.

@dafioram dafioram reopened this Oct 5, 2018

@bluegr

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Almost a year later...
From a casual review of the code, the changes look fine. Plus, there hasn't been any new activity on the engine since the pull request was opened. What kind of bugs are you referring to?

@dafioram

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Possibly run time. I tested it after the refactor, but I didn't play a significant amount of the game.

@bluegr

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

The engine is not yet complete, but this change is a step towards the right direction, as it provides a better separation of concerns.

I'm merging this, and if any regressions occur, they can be fixed in-tree

@bluegr bluegr merged commit 1ed2cd4 into scummvm:master Sep 11, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.