[FIXED] Accurate Display.sync()

Started by kappa, February 20, 2012, 22:52:04

Previous topic - Next topic

Riven

Quote from: kappa on March 26, 2012, 19:35:55
However its a valid optimisation and might be worth tweaking in.

I'd like to stress that dampening is impossible if the 'sum' is an aggregrated value (used for calculating 'avg'). It will leak time, and can't recover. Look very carefully at the code, the dampening is applied to the values in the slots, which means that the dampening will (slowly) disappear after 10 frames. This handles both erratic delays and poor resolution timers. Please don't optimize for optimizations-sake. The current code is correct and extremely fast. The 'correct' part is most important.

It can be a few nanoseconds faster, indeed, but at the expense of reliability and correctness! There are enough screwed up sync( ) implementations, I'd prefer to keep this implementation correct.

Don't get me wrong, dr_evil: if you think your implementation is better, taking into account a vast number of corner cases, I'd be very interested.

kappa

Agreed, happy with the way the code is now but yeh improvements are always welcome.

However as it stands unless there is some serious issue found I'd say this RFE is pretty much complete.

Simon Felix

Agreed, it won't make much difference. I proposed to have the sum in addition to the slots array. Just for completeness, here's the proposed implementation:

135        private static class RunningAvg {
136	                private final long[] slots;
                        private long sum;
137	                private int offset;
138	                
139	                private static final long DAMPEN_THRESHOLD = 10 * 1000L * 1000L; // 10ms
140	                private static final float DAMPEN_FACTOR = 0.9f; // don't change: 0.9f is exactly right!
141	
142	                public RunningAvg(int slotCount) {
143	                        this.slots = new long[slotCount];
144	                        this.offset = 0;
                                this.sum = 0;
145	                }
146	
147	                public void init(long value) {
                                this.sum = this.slots.length * value;
148	                        for (int i = 0; i < this.slots.length; i++) {
149	                                this.slots[i] = value;
150	                        }
151	                }
152	
153	                public void add(long value) {
                                this.sum += value - this.slots[this.offset];
154	                        this.slots[this.offset++] = value;
155	                        this.offset %= this.slots.length;
156	                }
157	
158	                public long avg() {
163	                        return this.sum / this.slots.length;
164	                }
165	                
166	                public void dampenForLowResTicker() {
167	                        if (this.avg() > DAMPEN_THRESHOLD) {
                                        this.sum = 0;
168	                                for (int i = 0; i < this.slots.length; i++) {
169	                                        this.sum += (this.slots[i] *= DAMPEN_FACTOR);
170	                                }
171	                        }
172	                }
Download Cultris II, the fastest Tetris clone from http://gewaltig.net/

Riven

You're basically tracking time using two seperate mechanisms.
Yes, it works, and it's marginably faster, but you trade maintainability.
I'd prefer KISS, until problems arise.

If the Running Average was wildly over 10 samples, I'd say great, as then it would matter, but in this case it's a very specialized piece of code, as proven by the appearance of the naughty .dampenForLowResTicker() method. :)

Simon Felix

Got very positive feedback from our playerbase regarding the newest code. Thanks a lot!
Download Cultris II, the fastest Tetris clone from http://gewaltig.net/