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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further fixes for samples/expert-game-of-life.rb #650

Merged
merged 3 commits into from Apr 16, 2014

Conversation

jasonrclark
Copy link
Member

This covers the last problems highlighted by #538 (although not all the problems with the same 馃槮)

Since style is not yet supported fully on Button, and it was only being used to effectively hide/show the buttons, I changed that over to using the hide and show methods more naturally.

I noted, however, once that was done, that we weren't getting the animation like you'd expect. The state of the rect elements that make up the grid were being updated, as resizing the window would jump to the new state, but it wasn't triggering on animate.

I concluded that we needed to have an element update in the RedrawingAspect when a style call is made to change an element, and that got us animating properly again. I had to be careful to only style when a cell needed a change, though (for reasons I'll get to next)

So with and without my style redrawing hook, this app is really slooooooow. I feel like we should call issue #538 done, though, with these changes, and enter another for performance tuning rather than drag it on before the alpha. Solving the perf issue could get quite involved.

Opinions on that plan?

Buttons don't properly support calling style with displacement values yet, but
the displacements were only to move things on/off screen, so just hide/show
instead.
The existing game of life code wasn't properly triggering element redraws when
the style of a cell changed states. This meant that clicking run would go, but
only show you the results when you forced a redraw by other means (i.e.
resizing the window).

This commit adds a redrawing trigger for the common `style` call and updates
the sample to be more cautious about style-setting to avoid masses of updates
all at once.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2fe36b7 on button_displace_style into 4f64bff on master.

@PragTob
Copy link
Member

PragTob commented Apr 16, 2014

I like the plan. We'd need to file an issue to revert the commit/changes to the sample once it's done. Oh wait no. Could you please do a expert-game-of-life-adjusted.rb and keep the original intact?

About redrawing, yeah I believe there is already an issue filed about that. Basically every rect would trigger a redraw now we need to batch those together/wait for a couple and then redraw. Would be a great performance improvement. Also we could get the check you make into the code... sooo many possibilities.

Shoes3 had a redrawing loop that'd just redraw everything every 30 seconds or so.

Performance tuning will be a great fun! :-D

EDIT: It's not redraw everything every 30 seconds but 30 times a second - excuse the slightly tired German ;)

@wasnotrice
Copy link
Member

Shoes3 had a redrawing loop that'd just redraw everything every 30 seconds or so.

Funny...my Shoes 3 redraws more frequently than that :P

@PragTob
Copy link
Member

PragTob commented Apr 16, 2014

Shoes3 had a redrawing loop that'd just redraw everything every 30 seconds or so.
Funny...my Shoes 3 redraws more frequently than that :P

Actually that's nothing I ever researched it's just something I've seen people write quite some times so I believe it :P

@wasnotrice
Copy link
Member

So...only twice a minute? Seems like we might want to redraw more frequently.

@wasnotrice
Copy link
Member

Is it feasible/desirable to allow for adding a condition in AfterDo/RedrawingAspect? For example, "redraw this element after #style, but only if #dirty?"?

def draw
@shoes_cell.style(fill: live? ? '#000' : '#fff')
fill = live? ? '#000000' : '#ffffff'
Copy link
Member

Choose a reason for hiding this comment

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

Did you change to 6-digit hex strings because the 3-digit ones didn't work? Let's make sure we file an issue for that if so.

Copy link
Member Author

Choose a reason for hiding this comment

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

3-digit worked fine for setting it, but looking it back up was returning me the 6-digit flavor. Since I needed to compare them to decide whether to set it or not, seemed simplest to change the values I was setting.

Not sure that this is a general issue, and with performance tuning we should be able to get to where this check isn't necessary at all.

Copy link
Member

Choose a reason for hiding this comment

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

I see the issue now. It's because you wanted to compare to Color#hex later. We can totally ignore this then. I just wanted to make sure you weren't also having to work around a Color issue as well :)

Copy link
Member Author

Choose a reason for hiding this comment

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

馃憤

@PragTob
Copy link
Member

PragTob commented Apr 16, 2014

Ouch.. herp derp me... that was supposed to read 30 times a second. Excuse me :P

It is feasible to add something like that would push it to rc1 though. I also thought about doing on the element side, e.g. it checks if the value actually needs to change and then calls a method like change_attribute and after_do would hook in there then. Depends on where we want to have the logic.

@wasnotrice
Copy link
Member

I agree, we can wait for rc1 to tackle this. (Edit: we can wait to tackle the performance issues)

@jasonrclark
Copy link
Member Author

@PragTob good idea about keeping the original. Will move my changes to and -adjusted version and call this good.

I couldn't find an existing issue about the redrawing performance. If anyone else knows which one it is, chime in, otherwise I might tally up our ideas about it and enter an issue when I have more than a few moments at the keyboard.

@wasnotrice
Copy link
Member

Maybe @PragTob was thinking about #579?

@PragTob
Copy link
Member

PragTob commented Apr 16, 2014

Nope I didn't - I believe I was thinking about some issue that was about making redraws more accurate. We did that (so it's close), but there is no one for batching/etc. (if that actually is the problem). So I guess there is no such issue.

So please go ahead @jasonrclark and create such an issue! :)

@jasonrclark
Copy link
Member Author

Created #651 and pushed commit to restore the original version and add an adjusted.

jasonrclark added a commit that referenced this pull request Apr 16, 2014
Further fixes for samples/expert-game-of-life.rb

Resolves #538
@jasonrclark jasonrclark merged commit 946169e into master Apr 16, 2014
@jasonrclark jasonrclark deleted the button_displace_style branch April 16, 2014 20:14
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