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

iPhone 6Plus weird overlap #4

Closed
Kowaio opened this issue Apr 23, 2015 · 17 comments · Fixed by #6
Closed

iPhone 6Plus weird overlap #4

Kowaio opened this issue Apr 23, 2015 · 17 comments · Fixed by #6

Comments

@Kowaio
Copy link

Kowaio commented Apr 23, 2015

Hello there,

First of all, thanks a lot for this layout, it really works well on my iPhone 6 device and it is really great to manage the horizontal alignment like that.

However, I'm contacting you today because while using the exact same code on the iPhone 6 Plus simulator, I've got a weird bug where cells overlap. I checked the code of course but can't seem to find what's causing that for this specific device, since it's working great on others. Maybe you had the same issue once and you can help me?

Here's some screenshot of the bug: http://cl.kowaio.me/image/2w1M1b3X3C13

About the code I call, I'm doing it in swift but like I told before, it's working great on other devices. Here's the code:

override func viewDidLoad() {
    super.viewDidLoad()

    // Update Collection View Flow Layout
    let layout = KTCenterFlowLayout()
    layout.minimumInteritemSpacing = 20.0
    layout.minimumLineSpacing = 10.0

    collectionView.collectionViewLayout = layout

}

and

func collectionView(collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, insetForSectionAtIndex section: Int) -> UIEdgeInsets {
    return UIEdgeInsets(top: 10.0, left: 20.0, bottom: 10.0, right: 20.0)
}

Thanks a lot!

My best,

@Kowaio
Copy link
Author

Kowaio commented Apr 23, 2015

After more research, I found that it seems to be linked to my items random size.

func collectionView(collectionView: UICollectionView, layout collectionViewLayout: UICollectionViewLayout, sizeForItemAtIndexPath indexPath: NSIndexPath) -> CGSize {
    let size = sizeArray["\(indexPath.row)"]!
    return CGSize(width: size.0, height: 50/*size.1*/)
}

By putting a height of 50 fixed, it works well, even on iPhone 6 Plus. I'll try and check why it's different and is working great with a random height for other devices, then.

If you have an input, feel free to jump in!

Thanks again,

@keighl
Copy link
Owner

keighl commented Apr 23, 2015

Hey @Kowaio, interesting. I'll try to replicate! What does your layout look like with the regular UICollectionViewFlowLayout?

@Kowaio
Copy link
Author

Kowaio commented Apr 23, 2015

Hey @keighl,

Once again, nice to meet you and thanks for jumping in! :-)

So I took two more screenshots. The first one (http://cl.kowaio.me/image/2e3J260h2w3p) is using your layout but with a fixed height (50) with the above code.

And the second one (http://cl.kowaio.me/image/2J1D2S0q2u46) without using your code but the standard flow layout (justified) when creating the Outlet, and with the random height.

About my randoms, it is completely random for the width, and for the height, I only randomize the font size and add a constant of 20 for more padding. And it is done in the viewDidLoad, so long before the collectionView to display cells.

My guess is that for some reason the random height is creating a weird overlap on the row where the items need to be spaced, but I don't know for sure. Moreover, like I said the only case where this doesn't appear, like in your demo, is with no random height.

Feel free to ask me if you need more info!

@keighl
Copy link
Owner

keighl commented Apr 23, 2015

Alright @Kowaio, I was able to replicate in the demo project with my 6+. I attached some screenshots here. It's probably something dumb in the frame adjustment logic of layoutAttributesForElementsInRect.

I'll do some debugging today, if I have time. Feel free to take a stab at it as well.

Thanks for reporting this!

center_layout

regular_flow_layout

@Kowaio
Copy link
Author

Kowaio commented Apr 23, 2015

Thanks a lot! And yes, sure, I'll try and find a proper fix too :-)

Thanks again

@Kowaio
Copy link
Author

Kowaio commented Apr 24, 2015

I checked a little bit this morning and indeed, it seems to come from the first loop, organizing rows by their middle Point. Since they don't have all the same middle point due to the random height, they're not correctly organized in the NSDictionary.

I updated the code with the following but didn't have time to test it much, neither in your demo sample. Keep me in touch, I'll get back to you if I manage to free some time today or during the week-end.

My best,

CGRect previousFrame = CGRectZero;
NSInteger row = 0;
for (UICollectionViewLayoutAttributes *itemAttributes in superAttributes)
{
    if( itemAttributes.frame.origin.x <=  CGRectGetMaxX(previousFrame) ) {
        row++;
    }

    NSNumber *key = @(row);

    if (!rowCollections[key])
        rowCollections[key] = [NSMutableArray new];

    [rowCollections[key] addObject:itemAttributes];

    previousFrame = itemAttributes.frame;
}

screenshot : http://cl.kowaio.me/image/1R1X0b2z1L3S

@Kowaio
Copy link
Author

Kowaio commented May 6, 2015

Hello,

My code doesn't seem to work well, actually. It is still occuring, but a lot less than before though. For some reason, it seems the cells displayed by cellForItemAtIndexPath from the collection view don't match those in the centered layout we're returning.

By any chance, did you have time to find the cause of it? I'm gonna check it a little bit more today, I have some spare time.

Thanks.

@keighl
Copy link
Owner

keighl commented May 6, 2015

Hey @Kowaio, thanks for digging deeper. Cells in the same row "should" have the same midY coord regardless of height.

I have a few minutes here now, gonna memoizing the rects, and check it against what the super class is returning in layoutAttributesForItemAtIndexPath

@keighl
Copy link
Owner

keighl commented May 6, 2015

Correction.... they should be visibly aligned. But the values can be ever so slightly different depending on height. That seems to be the issue. Trying some precision adjustment to fix it.

@keighl
Copy link
Owner

keighl commented May 6, 2015

@Kowaio Ok I think I have this ironed out. Try the variable-height-fix branch in your Podfile. It's working for me.

screen shot 2015-05-06 at 9 34 17 am

@Kowaio
Copy link
Author

Kowaio commented May 6, 2015

@keighl 👍
I just tested your code and it works really well. I knew it was in this area but didn't thought to play with the midY point.

Thanks again for you time and your fix.

@vincent0225
Copy link

@keighl
Thanks for this layout! It fit my need! But I found that this problem seems not completely fixed.
Attached a screenshot for your reference... Thanks a lot.
screen shot 2015-08-20 at 1 31 38 am

@vincent0225
Copy link

Similar Issue found on ipad2 simulator. The layout is fine at the beginning, but "New York" cell is overlap with other after I scroll the collection view.
screen shot 2015-08-20 at 1 36 47 am

@keighl
Copy link
Owner

keighl commented Sep 1, 2015

Thanks @vincent0225 I'll take a look when I can. Did you try on real 6+ as well?

@vincent0225
Copy link

img_0086
Yes, please find attached the screen shot.
The case happen when I scroll to bottom -> top -> bottom.
I think it may related to cell reuse?

@keighl
Copy link
Owner

keighl commented Sep 2, 2015

Probably not. It has to do with fractional points in the item frame. The way this layout works is by grouping items with the same middle Y coordinate (which is how the regular UICollectionViewFlowLayout lays out rows).

However, especially on 3x res devices, item center Ys may be +/- fractional points away from each other. Making it hard to group by visual row.

The existing fix for this is some additional logic that attempts to normalize close center Ys. See here

Also, it's only a problem when items have different heights. If all items in the row have the same height, it's fine.

@vincent0225
Copy link

Thanks a lot.
Do you have any suggestion for me about how to fix this case if the items have different height?
It's awesome if the cell have different size.

@keighl keighl reopened this Sep 27, 2015
@keighl keighl closed this as completed in 6ffaa6d May 14, 2016
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 a pull request may close this issue.

3 participants