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

Minor vehicle fixes #436

Merged
merged 6 commits into from Mar 22, 2015
Merged

Minor vehicle fixes #436

merged 6 commits into from Mar 22, 2015

Conversation

@scurest
Copy link
Contributor

@scurest scurest commented Mar 18, 2015

Some very minor fixes on the heels of #434.

2dfa0d5 adds a "stepping speed" to decouple the rate that characters do their left-middle-right-middle animation from their movement speed, but since it's actually the player's movement speed that's faster when riding a vehicle, an alternative would be to set all the vehicle's move speeds to 1/8 and move the switch to choose higher move speeds when riding vehicles to the line changed by e7880d8. I don't think the vehicles' move speed actually affect anything but their stepping animation does it?

Was everyone ok with 10455da?

scurest added 3 commits Mar 17, 2015
This fixes a bug where, when a vehicle which was not on the map
when the spritset was created is moved to the map, the vehicle
is invisible.
@fdelapena
Copy link
Contributor

@fdelapena fdelapena commented Mar 18, 2015

Was everyone ok with 10455da?

Looks good to me if this fixes that particular offcanvas issue unless there is a better approach.
@Zegeri did you have a planned alternate way to solve this in #434 as commented in the PR description?

@Zegeri
Copy link
Member

@Zegeri Zegeri commented Mar 18, 2015

I agree this is the simplest solution, I was thinking of refreshing the sprites of vehicles when SetVehicleLocation is used, like ChangeMapTileset does with tileset changes. But that would mean to have another vector of sprites just for the vehicles, not a pretty solution.

By the way, this is not the issue commented in the PR description.

Board a vehicle outside the borders of the map

That means to board a vehicle that it's in the current map, but outside the range of valid positions. For example, if the player is in (1,1) facing left and a ship is in (0,1) (not a valid position), the player should be able to get on the vehicle. This is how RPG_RT behaves.

@Ghabry
Copy link
Member

@Ghabry Ghabry commented Mar 18, 2015

I don't think such optimizations are worth the effort for 3 sprites. They won't induce a huge cpu penalty.

Another question is if the Draw-code of off-screen Sprites draws anything or just skips. That would be a general optimization that all sprites (including vehicles) profit from. No idea if we already do this.

@scurest
Copy link
Contributor Author

@scurest scurest commented Mar 19, 2015

Thanks Zegeri. Can I take out these lines too?

@Ghabry looks like a visibility test is the first thing out the door for sprite drawing, but they still participate in z-ordering and stuff. One more thing is the airship shadow is basically one or two more sprites in the spriteset too, and they'll have the same issue.

Also, miscellaneous whitespace fix.
@fdelapena
Copy link
Contributor

@fdelapena fdelapena commented Mar 19, 2015

About airship shadow: there is a blend of two semitransparent sprites from system.png.

I've prepared two red and blue squares there for testing:
default

You can compare them with these Cairo (pixman) operations pictures to check which blend operation is closer. If you swap the colors, the colour blend result will be different:
http://cairographics.org/operators/

There are some PIXMAN_OP_* operator examples in source code (bitmap.cpp).

@Zegeri
Copy link
Member

@Zegeri Zegeri commented Mar 20, 2015

@scurest Yeah, we won't need those lines.

@scurest
Copy link
Contributor Author

@scurest scurest commented Mar 21, 2015

Alright, if everyone's good, that's all the stuff I had for this branch. I have code for the airship shadow too but I'll do it in another PR.

@fdelapena
Copy link
Contributor

@fdelapena fdelapena commented Mar 21, 2015

Looks good to me, thanks 👍.

@Ghabry
Copy link
Member

@Ghabry Ghabry commented Mar 22, 2015

so this is rdy?

@scurest
Copy link
Contributor Author

@scurest scurest commented Mar 22, 2015

Yeah.

Ghabry added a commit that referenced this pull request Mar 22, 2015
@Ghabry Ghabry merged commit 9c2e2a0 into EasyRPG:master Mar 22, 2015
1 check passed
1 check passed
@fdelapena
default Merged build finished.
Details
@scurest scurest deleted the scurest:vehicles branch Sep 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants