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

Renderer refactoring - Renderables #3286

Merged
merged 25 commits into from
May 25, 2013

Conversation

pchote
Copy link
Member

@pchote pchote commented May 15, 2013

This streamlines the creation and drawing of Renderables, dropping a lot of accumlated cruft and adding support for alternative renderable types in the future (voxels, laser zaps, range circles).

Renderables have been generalized from additional Sprite state to an interface describing "something that can draw itself", positioned using world coordinates.

I've tested that the common cases display properly, but I there may be edge cases i've missed - resulting in offset sprites or incorrect draw orders.

@chrisforbes
Copy link
Member

This change makes the cache pretty upset :(

@cjshmyr
Copy link
Member

cjshmyr commented May 15, 2013

@chrisforbes : for the uninformed, explain?

On Wed, May 15, 2013 at 2:51 PM, Chris Forbes notifications@github.comwrote:

This change makes the cache pretty upset :(


Reply to this email directly or view it on GitHubhttps://github.com//pull/3286#issuecomment-17965372
.

@pchote
Copy link
Member Author

pchote commented May 16, 2013

@cjshmyr: He means that putting a pile of virtual function calls in the hot path of the renderer wasn't the smartest idea performance wise...
@chrisforbes: I've tweaked the patch to use an interface on structs instead.

@Mailaender
Copy link
Member

The Red Alert Destroyer and Cruiser turrets are mis-aligned.

@Mailaender
Copy link
Member

Red Alert Flak Truk turret is also mis-aligned.

@pchote
Copy link
Member Author

pchote commented May 16, 2013

Thanks for the report @Mailaender. Sounds like custom turret offsets are busted in general.

@pchote
Copy link
Member Author

pchote commented May 17, 2013

Updated to fix the turret offset issue.

@chrisforbes
Copy link
Member

👍 but I'd like @cjshmyr to take a look as well.

@cjshmyr
Copy link
Member

cjshmyr commented May 18, 2013

👍

@Mailaender
Copy link
Member

One glitch remaining: turrets are rendered on top of the RA weapon factory while they are spawning inside.

@pchote
Copy link
Member Author

pchote commented May 18, 2013

Whoops, that was introduced by the turret offset fix. Fixed in f7aca32.

@Mailaender
Copy link
Member

One minor rendering regression left: aircraft shadows are not drawn over regular buildings. They just vanish which looks slightly glitchy. Repair bay and heli pad work however.

@Mailaender
Copy link
Member

Could you also please move the flak truk turret a little more towards the front again. It seems to be just few pixels, but I noticed the difference and the gap looks strange.

@pchote
Copy link
Member Author

pchote commented May 23, 2013

Fixed. The flak truck turret was mispositioned by only 1px when facing south.

@Mailaender
Copy link
Member

The flak truk turret is rendered on top of the weapon factory and there are rendering glitches with boats, their turrets and naval production buildings also in regard to rendering order.

@pchote
Copy link
Member Author

pchote commented May 24, 2013

Fixed. These edge cases are a PITA, but I'd rather solve them now than later.

chrisforbes added a commit that referenced this pull request May 25, 2013
@chrisforbes chrisforbes merged commit 93c89a6 into OpenRA:bleed May 25, 2013
@Mailaender
Copy link
Member

I noticed that those edge cases exist in RA2.exe as shadows of trees are not properly rendered upon walls etc. and I also get SHP overlay glitches in Tiberian Sun with explosions so it seems we are already better than the original in this regard. Good work. 👍

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.

None yet

4 participants