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

QuadtreeClusteringService 구현 #38

Merged
merged 16 commits into from
Nov 28, 2020
Merged

Conversation

rnfxl92
Copy link
Collaborator

@rnfxl92 rnfxl92 commented Nov 28, 2020

구현내용

[feat], [clustering], [test]

  • 한 클러스터 영역 크기를 정해, 전체 BoundingBox(클러스터 해야되는 범위 전체)를 순서대로 순회하면서 Clustering한다.

논의사항

  • Coordinates MinMax 로 QuadTree BoundingBox 업데이트

    • 검색할때 시간이 줄지 않을까 예상하여 테스트했는데 유의미 한 속도차이는 나지 않았다...
    • 검색을 많이하면 달라질까..?
  • 진행중인 workItem Cancel이 안된다고 함..

    • 어떻게 할지 공부 더 필요

화면 (선택)

@rnfxl92 rnfxl92 added this to the Sprint 2 milestone Nov 28, 2020
@rnfxl92 rnfxl92 added this to To do in Sprint 2 via automation Nov 28, 2020
@rnfxl92 rnfxl92 self-assigned this Nov 28, 2020
@rnfxl92 rnfxl92 linked an issue Nov 28, 2020 that may be closed by this pull request
2 tasks
Comment on lines 33 to 35
// QuadTree boundingbox를 카메라 boundingbox로 했을 때와, minmax를 구해서 했을 때 비교하려고 만듬
self?.updateQuadTreeBoundingBox()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

주석 들여쓰기랑 코드 들여쓰기랑 똑같으면 좋을것같아요

Copy link
Collaborator

Choose a reason for hiding this comment

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

수정했습니다!

Comment on lines 76 to 77
var x = boundingBox.bottomLeft.x
var y = boundingBox.bottomLeft.y
Copy link
Collaborator

Choose a reason for hiding this comment

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

bottomLeftX, Y 는 어떨까요?
무시하셔도 좋아요 ㅎㅎ

Copy link
Collaborator

Choose a reason for hiding this comment

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

안 무시했습니다 !!😙

Comment on lines 81 to 83
defer {
x += clusterRegionWidth
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

defer가 어떤 역할을 하나요?
궁금해서 질문드립니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

defer 는 함수 마지막에 무조건 호출되는 함수로 알고있습니다 ~~

Copy link
Collaborator Author

@rnfxl92 rnfxl92 Nov 28, 2020

Choose a reason for hiding this comment

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

defer 블록 : 메소드에서 코드의 흐름과 상관 없이 가장 마지막에 실행되는 블록입니다. 중간에 continue하는 부분이 있는데 continue해도 이부분이 실행되야해서 사용했습니다.

- 주석 들여쓰기 수정
- x, y -> bottomLeftX, bottomLeftY 로 변경
Copy link
Collaborator

@beggu84 beggu84 left a comment

Choose a reason for hiding this comment

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

  1. BoundingBox 순서는 왜 바뀌었을 까요? 의논된 내용 이겠죠?

  2. 알고 계실 것 같지만, 두 어 개의 테스트가 실패하네요.

  3. 물론, 예상되는 값을 쉽게 계산하기 위해 그랬겠지만,
    테스트 케이스의 좌표들이 많고 다양하면 더 좋을 것 같습니다.

self.boundingBox = boundingBox
self.zoomLevel = zoomLevel
quadTree = QuadTree(boundingBox: boundingBox, nodeCapacity: Capacity.node)
insertCoordinates()
Copy link
Collaborator

Choose a reason for hiding this comment

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

바로 좌표들을 삽입해야하는 것 처럼 보이는데, insertCoordinatesAsync() 처럼 비동기로 처리한다는 뉘앙스를 주거나, 혹은 insertCoordinates 의 본문 내용을 바로 여기에 적어도 될 것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하! insertCoordinatesAsync로 변경했습니다 ! 감사합니다

let clusterRegionHeight: Double = (boundingBox.topRight.y - boundingBox.bottomLeft.y) / Double(heightCount)

var x = boundingBox.bottomLeft.x
var y = boundingBox.bottomLeft.y
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 두 문장을 이렇게도 가능 할까요?

var (x, y) = (boundingBox.bottomLeft.x, boundingBox.bottomLeft.y) 

Copy link
Collaborator

Choose a reason for hiding this comment

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

헉 몰랐네용 !! 꿀팁 감사합니다 ~~~ 😙

@@ -12,15 +12,19 @@ struct BoundingBox {
let topRight: Coordinate
let bottomLeft: Coordinate

var ratio: Double {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이부분은 Coordinate 내부에서 계산해서 리턴할 수 있도록 하는건 어떨까요

Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 수정했습니당!

@@ -32,10 +36,9 @@ struct BoundingBox {
}

func isOverlapped(with other: BoundingBox) -> Bool {
return (self.bottomLeft <= other.topRight && other.bottomLeft <= self.topRight)
self.bottomLeft <= other.topRight && other.bottomLeft <= self.topRight
Copy link
Collaborator

Choose a reason for hiding this comment

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

이부분도 BoundingBox 안에 넣어버리는게 어떨까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

앗 잘못봤네요. BoundingBox 내부 함수가 맞았네요 ㅎㅎ;;

private var zoomLevel: Double
private var clusterWidthCount: Int {
Int(min((zoomLevel / 3), 4))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: 혹시 let, var를 순서를 통일 시켜버리는건 어때요? ㅎㅎㅎ

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다 !!

}

private lazy var insertWorkItem = DispatchWorkItem { [weak self] in
self?.coordinates.forEach {
Copy link
Collaborator

@Seungeon-Kim Seungeon-Kim Nov 28, 2020

Choose a reason for hiding this comment

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

nit:
guard let self = self else { return } 하면 self를 옵셔널 체이닝을 안붙일 수 있어요
[unowned self]를 사용할 수 도 있어요

[참고]
https://usinuniverse.bitbucket.io/blog/self.html

Copy link
Collaborator

@eunjeongS2 eunjeongS2 Nov 28, 2020

Choose a reason for hiding this comment

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

넵 guard 문으로 바꿨습니당 ! 좋은 것같아요 감사합니다 ~~~


private func insertCoordinates() {
queue.async(execute: insertWorkItem)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

굳이 함수화한 이유가 있나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

이건 제 취향같은데 여러 함수가 불리는 곳에서는 로직을 바로 쓰지 않는 것 같습니다,, 많이 별론가용??

Copy link
Collaborator

Choose a reason for hiding this comment

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

음... 아니면 차라리 파라미터로 workItem을 파라미터로 넘겨서 사용하는건 어떨까요

Copy link
Collaborator

Choose a reason for hiding this comment

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

insertCoordinatesAsync 로 이름을 변경해서 괜찮은 것 같아요 !!

Copy link
Collaborator

Choose a reason for hiding this comment

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

취존합니다.

@eunjeongS2
Copy link
Collaborator

@beggu84
앗 트리에 삽입하는 순서를 바꿔놓고 테스트를 안돌려봤네요 ㅠ__ㅠ
고쳐서 다시 올렸습니다 !!

Sprint 2 automation moved this from To do to In progress Nov 28, 2020
@eunjeongS2 eunjeongS2 merged commit 65a7c4d into develop Nov 28, 2020
Sprint 2 automation moved this from In progress to Done Nov 28, 2020
@eunjeongS2 eunjeongS2 deleted the feature/quadtree-service branch December 3, 2020 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

QuadTreeManager 구현
5 participants